Skip to content

particle support limit velocity over lifetime#2925

Open
hhhhkrx wants to merge 49 commits intogalacean:dev/2.0from
hhhhkrx:feat/limitParticle
Open

particle support limit velocity over lifetime#2925
hhhhkrx wants to merge 49 commits intogalacean:dev/2.0from
hhhhkrx:feat/limitParticle

Conversation

@hhhhkrx
Copy link
Contributor

@hhhhkrx hhhhkrx commented Mar 16, 2026

概述

为粒子系统新增 LimitVelocityOverLifetime 模块,通过 WebGL2 Transform Feedback 实现 GPU 端逐帧速度限制(dampen)和阻力(drag)模拟。

架构设计

三层 Transform Feedback 架构:

  • TransformFeedback — WebGL2 TF 对象封装(bind/unbind/begin/end)
  • TransformFeedbackPrimitive — ping-pong 双缓冲 + VAO 管理
  • TransformFeedbackSimulator — 通用 TF 模拟器(shader 编译、program 缓存、simulate 调度)
  • ParticleTransformFeedbackSimulator — 粒子专用,继承通用模拟器

平台抽象:

  • IPlatformTransformFeedback / IPlatformTransformFeedbackPrimitive — 核心接口
  • GLTransformFeedback / GLTransformFeedbackPrimitive — WebGL2 实现

TF Shader 算法(particle_feedback_simulation.glsl)

每帧对每个存活粒子执行四步:

  1. 力与速度叠加 — Gravity 和 FOL(含 world-space)转为 local 后累加到持久速度;VOL 作为瞬时叠加层不持久化
  2. 限速(Dampen) — VOL 统一投影到 LVL 目标空间,总速度超过 limit 时按 dampen 比例削减,只修改持久基础速度
  3. 阻力(Drag) — 同样在 LVL 目标空间计算总速度,每帧减去 drag * dt,支持 multiplyBySize / multiplyByVelocity
  4. 位置积分 — Local 模式在本地空间积分,World 模式在世界空间积分

Bug 修复

问题 修复
VOL 使用 curVOL - prevVOL 计算,常量模式下结果为零 改为使用完整瞬时速度值
World-space FOL 未累加到持久速度,位移为线性而非二次方 和 gravity 同样处理,转 local 后累加
TF 初始位置 bake 为世界坐标,Local 模式下粒子不跟随发射器 改为存储本地空间,渲染时 rotateToWorld + worldOffset
Stretched billboard 重复计算 localVelocity 正确拆分 local/world velocity,渲染时重新计算 VOL 叠加
VOL.space 与 LVL.space 不一致时 VOL 绕过限速 预算 volAsLocal/volAsWorld,统一投影到目标空间
Drag 单曲线模式与未初始化的 MinCurve 做 mix 添加 RENDERER_LVL_DRAG_IS_RANDOM_TWO 宏保护
WebGL1 下启用 LVL 静默失效但 getter 返回 true 添加 WebGL2 检查,不支持时直接 return

其他改动

  • ShaderPass.ts — 拆分 ShaderLab / 非 ShaderLab 编译路径,使手写 GLSL(TF shader)能正确编译
  • ShaderFactory.ts — 新增 compilePlatformSource 统一非 ShaderLab shader 编译流程
  • 移除解析路径的旧 drag 代码(getStartPosition / u_DragConstant),drag 现在仅在 TF 模式下生效
  • 更新 e2e 截图 + 新增 limitVelocity 测试用例

Summary by CodeRabbit

  • New Features

    • Limit Velocity Over Lifetime particle module: per-axis limits, dampening, drag and size/velocity multipliers for finer particle control.
    • GPU-driven particle updates (transform-feedback) for improved particle performance and fidelity.
    • New end-to-end particle case demonstrating the effect and texture-based additive particle material.
  • Tests

    • Comprehensive unit tests for the new module and an end-to-end test case added.

@hhhhkrx hhhhkrx self-assigned this Mar 16, 2026
@coderabbitai
Copy link

coderabbitai bot commented Mar 16, 2026

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (1)
  • e2e/fixtures/originImage/Particle_particleRenderer-limitVelocity.jpg is excluded by !**/*.jpg

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: c6e9a83f-40af-49d6-90a5-4a66cdcbeb63

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Walkthrough

Adds GPU transform‑feedback particle simulation, a new LimitVelocityOverLifetime particle module, WebGL2 transform‑feedback platform/primitive implementations, shader compilation and ShaderLab path updates, buffer/layout additions, generator/renderer TF integration, unit and e2e tests, and related RHI exports.

Changes

Cohort / File(s) Summary
LimitVelocity module & exports
packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts, packages/core/src/particle/enums/ParticleRandomSubSeeds.ts, packages/core/src/particle/index.ts
New LimitVelocityOverLifetimeModule with scalar/per‑axis limits, drag/dampen, shader macros/uniforms; added random seed enum entry and exported module.
Particle generator & TF integration
packages/core/src/particle/ParticleGenerator.ts, packages/core/src/particle/ParticleRenderer.ts, packages/core/src/particle/ParticleBufferUtils.ts, packages/core/src/particle/enums/attributes/ParticleFeedbackVertexAttribute.ts
Integrates limit module and transform‑feedback paths into generator/renderer; adds feedback vertex/instance layouts, TF toggle, TF buffer/instance binding handling, emission TF initialization, and shader‑data propagation.
Transform‑feedback simulators & primitives
packages/core/src/particle/ParticleTransformFeedbackSimulator.ts, packages/core/src/graphic/TransformFeedbackSimulator.ts, packages/core/src/graphic/TransformFeedbackPrimitive.ts
New TF simulation pipeline with program caching, uniform upload, ping‑pong read/write buffers, draw lifecycle, and per‑particle initialization APIs.
Platform TF API & WebGL implementations
packages/core/src/graphic/TransformFeedback.ts, packages/rhi-webgl/src/GLTransformFeedback.ts, packages/rhi-webgl/src/GLTransformFeedbackPrimitive.ts, packages/rhi-webgl/src/WebGLGraphicDevice.ts
Platform TransformFeedback and TFPrimitive abstractions and WebGL2 implementations; device factory methods, rasterizer‑discard controls, and shader program state invalidation.
Rendering hardware interfaces
packages/core/src/renderingHardwareInterface/IPlatformTransformFeedback.ts, packages/core/src/renderingHardwareInterface/IPlatformTransformFeedbackPrimitive.ts, packages/core/src/renderingHardwareInterface/index.ts
New/extended RHI interfaces for transform‑feedback operations and primitives; re‑exported from RHI index.
Shader pipeline & compilation
packages/core/src/shader/ShaderProgram.ts, packages/core/src/shader/ShaderPass.ts, packages/core/src/shaderlib/ShaderFactory.ts
ShaderProgram accepts transformFeedbackVaryings; ShaderPass adds ShaderLab program path and unified macro parsing; ShaderFactory.compilePlatformSource builds platform GLSL with macros/versioning.
Particle shader modules
packages/core/src/shaderlib/particle/index.ts
Exports new GLSL modules: limit_velocity_over_lifetime_module and particle_feedback_simulation.
Buffer / copy API changes
packages/core/src/graphic/Buffer.ts, packages/core/src/renderingHardwareInterface/IPlatformBuffer.ts, packages/rhi-webgl/src/GLBuffer.ts
Exposes platform buffer field and adds copyFromBuffer API to Buffer/IPlatformBuffer/GLBuffer to support GPU buffer-to-buffer copies used by TF resizing/copies.
Particle buffer utils & attributes
packages/core/src/particle/ParticleBufferUtils.ts, packages/core/src/particle/enums/attributes/ParticleFeedbackVertexAttribute.ts
Adds feedbackVertexStride, feedback vertex/instance element arrays and feedback attribute enum for TF layouts.
Particle TF usage in renderer
packages/core/src/particle/ParticleRenderer.ts, packages/core/src/particle/ParticleTransformFeedbackSimulator.ts
Renderer now invokes generator TF update when enabled; new ParticleTransformFeedbackSimulator wraps TF update flow and exposes readBinding for render.
Tests & E2E
tests/src/core/particle/LimitVelocityOverLifetime.test.ts, tests/src/core/CloneUtils.test.ts, e2e/config.ts, e2e/case/particleRenderer-limitVelocity.ts
Adds comprehensive unit tests for the new module and clone/remap logic, large CloneUtils tests, and an e2e case that exercises the LimitVelocity emitter and TF path.
Cloning API simplifications
multiple packages/core/src/... and packages/ui/src/... files (many _cloneTo signatures, Component/Entity additions, CloneManager/CloneUtils/ComponentCloner changes)
Widespread clone/remap refactor: new _remap hooks, simplified _cloneTo signatures, CloneManager/CloneUtils overhaul, and updated component clone interfaces.
Misc / packaging
package.json, packages/*/package.json, packages/core/src/graphic/enums/BufferBindFlag.ts
Version bumps across packages and minor JSDoc formatting edits.

Sequence Diagram(s)

sequenceDiagram
    participant App
    participant ParticleRenderer
    participant ParticleGenerator
    participant TFSimulator
    participant ShaderProgram
    participant WebGLContext

    App->>ParticleRenderer: update(deltaTime)
    ParticleRenderer->>ParticleGenerator: _update(shaderData, deltaTime)
    alt Transform Feedback enabled
        ParticleGenerator->>TFSimulator: update(shaderData, particleCount, firstActive, firstFree, deltaTime)
        TFSimulator->>ShaderProgram: compile/retrieve(program with feedback varyings)
        TFSimulator->>WebGLContext: bind transform feedback & buffers
        WebGLContext->>TFSimulator: beginTransformFeedback(primitive)
        TFSimulator->>WebGLContext: drawArrays(firstActive, count)
        WebGLContext-->>TFSimulator: captured feedback (position, velocity)
        WebGLContext->>TFSimulator: endTransformFeedback()
        TFSimulator->>TFSimulator: swap read/write buffers
        TFSimulator-->>ParticleGenerator: provide readBinding for render
    end
    ParticleRenderer->>WebGLContext: render using instance buffers (may use TF read binding)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~65 minutes

Possibly related issues

Possibly related PRs

Suggested labels

ignore for release

Suggested reviewers

  • luzhuang

Poem

🐰 I hop where buffers hum and glow,

I tuck the speed so sprites don't blow,
On GPU winds I clip and bind,
Swap, dampen, drift — a measured kind,
A carrot-coded limit show.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main feature: adding particle Limit Velocity Over Lifetime support, which is the primary objective of the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Mar 16, 2026

Codecov Report

❌ Patch coverage is 83.56725% with 281 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.32%. Comparing base (57fdddb) to head (0ffa4cf).
⚠️ Report is 3 commits behind head on dev/2.0.

Files with missing lines Patch % Lines
e2e/case/particleRenderer-limitVelocity.ts 0.00% 89 Missing and 1 partial ⚠️
...article/modules/LimitVelocityOverLifetimeModule.ts 81.79% 83 Missing ⚠️
packages/core/src/particle/ParticleGenerator.ts 78.34% 34 Missing ⚠️
packages/core/src/clone/CloneManager.ts 66.21% 25 Missing ⚠️
e2e/config.ts 0.00% 7 Missing ⚠️
...src/particle/ParticleTransformFeedbackSimulator.ts 93.57% 7 Missing ⚠️
...ges/core/src/graphic/TransformFeedbackSimulator.ts 95.77% 6 Missing ⚠️
packages/core/src/shader/ShaderProgram.ts 64.70% 6 Missing ⚠️
packages/core/src/graphic/TransformFeedback.ts 93.42% 5 Missing ⚠️
...ges/core/src/graphic/TransformFeedbackPrimitive.ts 96.71% 5 Missing ⚠️
... and 4 more
Additional details and impacted files
@@             Coverage Diff             @@
##           dev/2.0    #2925      +/-   ##
===========================================
+ Coverage    77.29%   77.32%   +0.02%     
===========================================
  Files          884      895      +11     
  Lines        96638    98110    +1472     
  Branches      9509     9627     +118     
===========================================
+ Hits         74697    75862    +1165     
- Misses       21777    22083     +306     
- Partials       164      165       +1     
Flag Coverage Δ
unittests 77.32% <83.56%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@e2e/case/particleRenderer-limitVelocity.ts`:
- Around line 27-65: The test never uses the imported E2E helpers so the
screenshot harness may capture before particles/textures finish; at the end of
createScalarLimitParticle add calls to updateForE2E(engine, 500) and
initScreenshot(engine, camera) so the engine has time to settle and the camera
frame is registered before the reference screenshot is taken — locate the calls
inside the async particle setup sequence (referencing createScalarLimitParticle,
engine and camera) and append updateForE2E and initScreenshot accordingly.

In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts`:
- Around line 254-269: _isRandomMode currently only considers
_limitX/_limitY/_limitZ and thus returns false when drag uses
TwoConstants/TwoCurves, causing ParticleGenerator to skip per-particle randoms
(Random2.w) and produce deterministic drag; update _isRandomMode (and the
equivalent checks in the ranges 274-323 and 427-448) to include the module's
drag random modes as well (i.e., check the drag-related curve/mode fields used
by _uploadDrag for TwoConstants and TwoCurves) so the module-wide "needs
per-particle random" decision returns true whenever drag is in a random mode and
ParticleGenerator will allocate Random2.w accordingly. Ensure you reference and
update the same logic in the _isRandomMode helper(s) and harmonize it with
_uploadDrag's accepted modes so behavior is consistent.
- Around line 360-425: The function _uploadSeparateAxisLimits currently coerces
mixed per-axis modes to the constant path; update it to handle each axis
individually by checking limitX.mode, limitY.mode, limitZ.mode separately and
uploading the correct representation per axis: use
LimitVelocityOverLifetimeModule._limitXMaxCurveProperty/_limitXMinCurveProperty
(and Y/Z equivalents) when an axis is Curve/TwoCurves, and write the
corresponding component into
LimitVelocityOverLifetimeModule._limitMaxConstVecProperty/_limitMinConstVecProperty
for Constant/TwoConstants axes; set per-axis random flags and mode macros
(introduce or reuse per-axis macros if needed instead of the single
_limitCurveModeMacro/_limitConstantModeMacro) so the shader can distinguish
which axes are curves vs constants, and ensure _uploadSeparateAxisLimits returns
the appropriate macros (modeMacro/randomMacro) reflecting per-axis state rather
than forcing all-constant fallback.

In `@tests/src/core/particle/LimitVelocityOverLifetime.test.ts`:
- Around line 216-239: The tests override the global performance.now in multiple
places (e.g., inside the "enabling module triggers shader update without error"
case that manipulates engine._vSyncCount and engine._time._lastSystemTime and
calls engine.update()), but never restore it; capture the original
performance.now before replacing it and ensure you restore it in a finally block
or an afterEach hook so the global clock is returned to its original function
after each smoke test; update all occurrences that set performance.now
(including the other similar test blocks) to follow this pattern so later specs
are not affected by the mocked clock.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e9b43c5e-3212-4921-8132-1ec2c353b844

📥 Commits

Reviewing files that changed from the base of the PR and between 1efbfe2 and 3594b58.

⛔ Files ignored due to path filters (3)
  • packages/core/src/shaderlib/extra/particle.vs.glsl is excluded by !**/*.glsl
  • packages/core/src/shaderlib/particle/force_over_lifetime_module.glsl is excluded by !**/*.glsl
  • packages/core/src/shaderlib/particle/limit_velocity_over_lifetime_module.glsl is excluded by !**/*.glsl
📒 Files selected for processing (8)
  • e2e/case/particleRenderer-limitVelocity.ts
  • e2e/config.ts
  • packages/core/src/particle/ParticleGenerator.ts
  • packages/core/src/particle/enums/ParticleRandomSubSeeds.ts
  • packages/core/src/particle/index.ts
  • packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts
  • packages/core/src/shaderlib/particle/index.ts
  • tests/src/core/particle/LimitVelocityOverLifetime.test.ts

Comment on lines +216 to +239
it("enabling module triggers shader update without error", function () {
const lvl = particleRenderer.generator.limitVelocityOverLifetime;
lvl.enabled = true;
lvl.limit = new ParticleCompositeCurve(5);
lvl.dampen = 0.8;
lvl.drag = new ParticleCompositeCurve(0.5);

// Should not throw when updating shader data
particleRenderer.generator.play();
expect(() => {
//@ts-ignore
engine._vSyncCount = Infinity;
//@ts-ignore
engine._time._lastSystemTime = 0;
let times = 0;
performance.now = function () {
times++;
return times * 100;
};
for (let i = 0; i < 10; ++i) {
engine.update();
}
}).to.not.throw();
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Restore performance.now after these smoke tests.

Line 231, Line 257, and Line 282 overwrite the global clock and never restore it. That leaks into later specs and makes the browser suite order-dependent. Save the original function and put it back in finally or afterEach.

Suggested pattern
+    const originalNow = performance.now;
     expect(() => {
-      performance.now = function () {
-        times++;
-        return times * 100;
-      };
-      for (let i = 0; i < 10; ++i) {
-        engine.update();
-      }
+      try {
+        performance.now = function () {
+          times++;
+          return times * 100;
+        };
+        for (let i = 0; i < 10; ++i) {
+          engine.update();
+        }
+      } finally {
+        performance.now = originalNow;
+      }
     }).to.not.throw();

Also applies to: 241-265, 267-290

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/src/core/particle/LimitVelocityOverLifetime.test.ts` around lines 216 -
239, The tests override the global performance.now in multiple places (e.g.,
inside the "enabling module triggers shader update without error" case that
manipulates engine._vSyncCount and engine._time._lastSystemTime and calls
engine.update()), but never restore it; capture the original performance.now
before replacing it and ensure you restore it in a finally block or an afterEach
hook so the global clock is returned to its original function after each smoke
test; update all occurrences that set performance.now (including the other
similar test blocks) to follow this pattern so later specs are not affected by
the mocked clock.

GuoLei1990 and others added 2 commits March 17, 2026 02:44
- Implement per-frame TF-based particle simulation matching Unity's
  ClampVelocityModule (dampen, drag, execution order, world space handling)
- Add TransformFeedbackPrimitive for reusable ping-pong buffer/VAO management
- Add TransformFeedback/GLTransformFeedback platform abstraction layer
- Remove inaccurate 8-step approximation fallback (WebGL1 ignores module)
- Optimize TF pass to only process alive particles via bindBufferRange
- Optimize instance buffer upload with Discard mode for TF

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use VertexElement instead of custom TFAttributeDescriptor
- Auto-detect VAO rebuild via _lastProgramId (no manual markVAODirty)
- Extract attribute descriptors as static readonly arrays
- Remove unnecessary comments and simplify code

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

♻️ Duplicate comments (3)
e2e/case/particleRenderer-limitVelocity.ts (1)

27-27: ⚠️ Potential issue | 🟡 Minor

Hook this case into the screenshot harness.

initScreenshot and updateForE2E are still unused, so the capture can happen before the texture load and first burst settle. Please call updateForE2E(engine, 500) and initScreenshot(engine, camera) after the particle setup completes, even if that means threading camera into createScalarLimitParticle().

Also applies to: 63-65, 68-145

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@e2e/case/particleRenderer-limitVelocity.ts` at line 27, After the particle
setup completes, hook the test into the screenshot harness by calling
updateForE2E(engine, 500) and then initScreenshot(engine, camera); if camera
isn't available where you build the particle case, thread the camera through
(e.g. add a camera parameter to createScalarLimitParticle or return the camera
from the setup) so you can call initScreenshot(engine, camera) immediately after
setup; apply the same change to the other similar cases referenced (the blocks
that import initScreenshot/updateForE2E).
packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts (2)

256-270: ⚠️ Potential issue | 🟠 Major

Treat randomized drag as a random-mode configuration.

_uploadDrag() supports TwoConstants and TwoCurves, but _isRandomMode() still only checks the limit curves. In separate-axes mode it also requires every axis to be randomized. The module-level random check should flip on whenever any limit axis or drag uses a randomized mode; otherwise the generator skips the per-particle random this shader path needs and drag becomes deterministic.

Also applies to: 429-448

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts` around
lines 256 - 270, The _isRandomMode() function currently only inspects
_limitX/_limitY/_limitZ modes (and requires all axes randomized when
_separateAxes is true), but it must also treat randomized drag as random-mode so
the per-particle random shader path is enabled; update _isRandomMode() to return
true if any of _limitX/_limitY/_limitZ is in ParticleCurveMode.TwoConstants or
TwoCurves OR if the drag configuration inspected by _uploadDrag() is in
TwoConstants/TwoCurves (and when _separateAxes is true, consider each axis ORed
rather than requiring all three), ensuring the method references the same drag
mode checks used by _uploadDrag() so randomized drag flips the random-mode flag;
apply the same fix to the other _isRandomMode() copy around the 429-448 area.

362-424: ⚠️ Potential issue | 🟠 Major

Don't silently fall back to constants for mixed per-axis modes.

Anything other than “all curves” or “all constants” drops into the constant upload branch. A setup like limitX = Curve, limitY = Constant, limitZ = TwoConstants loses the authored curve/random data instead of matching the per-axis configuration. Either reject mixed setups or upload each axis independently.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts` around
lines 362 - 424, In _uploadSeparateAxisLimits detect and handle each axis mode
individually instead of falling back to the “all curves” vs “all constants”
branches: inspect limitX.limitY.limitZ modes and for each axis upload the
appropriate data (for curves use _limit{X|Y|Z}MaxCurveProperty and
_limit{X|Y|Z}MinCurveProperty when TwoCurves, for constants use
_limitMaxConstVecProperty/_limitMinConstVecProperty but only set the
corresponding component), set modeMacro to reflect per-axis curve/constant usage
(or compute a composite shader macro logic) and set randomMacro only if any axis
is random (TwoCurves/TwoConstants) while preserving each axis’s authored values;
modify _uploadSeparateAxisLimits, the uses of
LimitVelocityOverLifetimeModule._limitXMaxCurveProperty/_limitXMinCurveProperty/_limitY.../_limitZ...
and the constant vector packing logic to write per-component values rather than
globally switching branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/core/src/graphic/TransformFeedbackPrimitive.ts`:
- Around line 89-105: updateVAOs currently skips rebuilding VAOs when only
program.id matches _lastProgramId, causing stale VAO bindings if the underlying
buffers are reallocated; update the cache key logic in updateVAOs (and related
fields) to include the current transform buffers (this._readBuffer and
this._writeBuffer) so the VAOs are recreated when either buffer changes: compare
buffer identity (e.g., buffer.id or the buffer object references) against stored
lastReadBuffer/lastWriteBuffer (or store a composite _lastKey) and call
_deleteVAOs()/_createVAO(...) whenever program.id or either buffer has changed,
then update those stored buffer ids/refs along with _lastProgramId.
- Around line 58-84: resize() must reset the ping-pong state so an odd number of
prior swap() calls doesn't leave _useA false and cause read/write to the same
buffer; after recreating and assigning _readBuffer and _writeBuffer in
TransformFeedbackPrimitive.resize(), set the ping-pong flag (this._useA) to a
known side (e.g., true) and ensure the current render binding
(_renderBufferBinding) points at the new read buffer so subsequent draw()/swap()
behavior is consistent with the new buffers.

In `@packages/core/src/particle/ParticleGenerator.ts`:
- Around line 625-651: When enabling TF at runtime in _setTFMode, resizing the
transform feedback buffers is insufficient because existing live particles'
state must be copied into TF storage; update _setTFMode so after
creating/resizing this._transformFeedback (ParticleTransformFeedbackSimulator)
you backfill the TF buffers from the current CPU/GPU particle state (e.g. read
the geometry/attribute buffers or the particle system's CPU arrays for position,
velocity, life, etc.) into the transform feedback buffers before enabling the TF
macro (renderer.shaderData.enableMacro(ParticleGenerator._tfModeMacro)),
ensuring the first TF update sees correct data; keep the call to
_reorganizeGeometryBuffers() but perform the backfill immediately after resize
and before any TF-driven update.
- Around line 1023-1036: The world-space TF init currently uses identity
rotation; instead read the captured a_SimulationWorldRotation from the instance
buffer (stored at offset+30) when main.simulationSpace !==
ParticleSimulationSpace.Local and pass that quaternion into _initTFParticle so
the emitted shape position is rotated the same way as the non-TF path; update
the call sites that currently set qx/qy/qz/qw to identity to extract the
quaternion from the instance buffer (or forward it) and ensure _initTFParticle
consumes that quaternion rather than assuming identity, keeping the existing
local-space branch that uses transform.worldRotationQuaternion.

In `@packages/core/src/shader/ShaderProgram.ts`:
- Around line 267-274: The Transform Feedback setup in ShaderProgram currently
casts gl to WebGL2RenderingContext without validation; update the block in the
method that handles transformFeedbackVaryings to first assert that the context
is WebGL2 (e.g., using a type guard like "gl instanceof WebGL2RenderingContext"
or checking an existing "isWebGL2" flag) and early-return if it's not WebGL2,
then call (<WebGL2RenderingContext>gl).transformFeedbackVaryings only when that
guard passes to avoid unsafe casts and runtime errors for future callers.

In `@packages/rhi-webgl/src/GLTransformFeedback.ts`:
- Around line 13-17: The GLTransformFeedback constructor calls
createTransformFeedback() on rhi.gl without ensuring it's a WebGL2 context,
causing a TypeError for WebGL1; update the GLTransformFeedback constructor to
check that rhi.gl is an instance of WebGL2RenderingContext (or that
rhi.supportsTransformFeedback/WebGL2 flag is true) before calling
createTransformFeedback(), and if not supported either set _glTransformFeedback
to null/undefined and mark the instance as unsupported or throw a clear error;
make sure callers via WebGLGraphicDevice.createPlatformTransformFeedback(),
TransformFeedback, and TransformFeedbackPrimitive handle the unsupported case
consistently.

---

Duplicate comments:
In `@e2e/case/particleRenderer-limitVelocity.ts`:
- Line 27: After the particle setup completes, hook the test into the screenshot
harness by calling updateForE2E(engine, 500) and then initScreenshot(engine,
camera); if camera isn't available where you build the particle case, thread the
camera through (e.g. add a camera parameter to createScalarLimitParticle or
return the camera from the setup) so you can call initScreenshot(engine, camera)
immediately after setup; apply the same change to the other similar cases
referenced (the blocks that import initScreenshot/updateForE2E).

In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts`:
- Around line 256-270: The _isRandomMode() function currently only inspects
_limitX/_limitY/_limitZ modes (and requires all axes randomized when
_separateAxes is true), but it must also treat randomized drag as random-mode so
the per-particle random shader path is enabled; update _isRandomMode() to return
true if any of _limitX/_limitY/_limitZ is in ParticleCurveMode.TwoConstants or
TwoCurves OR if the drag configuration inspected by _uploadDrag() is in
TwoConstants/TwoCurves (and when _separateAxes is true, consider each axis ORed
rather than requiring all three), ensuring the method references the same drag
mode checks used by _uploadDrag() so randomized drag flips the random-mode flag;
apply the same fix to the other _isRandomMode() copy around the 429-448 area.
- Around line 362-424: In _uploadSeparateAxisLimits detect and handle each axis
mode individually instead of falling back to the “all curves” vs “all constants”
branches: inspect limitX.limitY.limitZ modes and for each axis upload the
appropriate data (for curves use _limit{X|Y|Z}MaxCurveProperty and
_limit{X|Y|Z}MinCurveProperty when TwoCurves, for constants use
_limitMaxConstVecProperty/_limitMinConstVecProperty but only set the
corresponding component), set modeMacro to reflect per-axis curve/constant usage
(or compute a composite shader macro logic) and set randomMacro only if any axis
is random (TwoCurves/TwoConstants) while preserving each axis’s authored values;
modify _uploadSeparateAxisLimits, the uses of
LimitVelocityOverLifetimeModule._limitXMaxCurveProperty/_limitXMinCurveProperty/_limitY.../_limitZ...
and the constant vector packing logic to write per-component values rather than
globally switching branches.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 135872d8-619c-4b54-83ae-4a092ee18b8f

📥 Commits

Reviewing files that changed from the base of the PR and between 272bf29 and 2193eb7.

⛔ Files ignored due to path filters (4)
  • packages/core/src/shaderlib/extra/particle.vs.glsl is excluded by !**/*.glsl
  • packages/core/src/shaderlib/particle/limit_velocity_over_lifetime_module.glsl is excluded by !**/*.glsl
  • packages/core/src/shaderlib/particle/particle_common.glsl is excluded by !**/*.glsl
  • packages/core/src/shaderlib/particle/particle_transform_feedback_update.glsl is excluded by !**/*.glsl
📒 Files selected for processing (16)
  • e2e/case/particleRenderer-limitVelocity.ts
  • packages/core/src/graphic/Buffer.ts
  • packages/core/src/graphic/TransformFeedback.ts
  • packages/core/src/graphic/TransformFeedbackPrimitive.ts
  • packages/core/src/graphic/enums/BufferBindFlag.ts
  • packages/core/src/particle/ParticleGenerator.ts
  • packages/core/src/particle/ParticleRenderer.ts
  • packages/core/src/particle/ParticleTransformFeedbackSimulator.ts
  • packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts
  • packages/core/src/renderingHardwareInterface/IPlatformTransformFeedback.ts
  • packages/core/src/renderingHardwareInterface/index.ts
  • packages/core/src/shader/ShaderProgram.ts
  • packages/core/src/shaderlib/particle/index.ts
  • packages/rhi-webgl/src/GLBuffer.ts
  • packages/rhi-webgl/src/GLTransformFeedback.ts
  • packages/rhi-webgl/src/WebGLGraphicDevice.ts
✅ Files skipped from review due to trivial changes (1)
  • packages/core/src/graphic/enums/BufferBindFlag.ts

Comment on lines +267 to +274
// Set Transform Feedback varyings before linking (WebGL2 only)
if (transformFeedbackVaryings?.length) {
(<WebGL2RenderingContext>gl).transformFeedbackVaryings(
program,
transformFeedbackVaryings,
(<WebGL2RenderingContext>gl).INTERLEAVED_ATTRIBS
);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify all ShaderProgram callsites that pass a 4th argument are WebGL2-gated.
rg -nP --type=ts -C3 'new\s+ShaderProgram\s*\([^,]+,\s*[^,]+,\s*[^,]+,\s*[^)]+\)'
rg -nP --type=ts -C3 '\bisWebGL2\b|transformFeedbackVaryings'

Repository: galacean/engine

Length of output: 44452


Consider adding WebGL2 validation in the Transform Feedback varyings setup.

The code at lines 268–273 unconditionally casts gl to WebGL2RenderingContext. While the only known call site in ParticleTransformFeedbackSimulator.ts:168 is already guarded with if (!engine._hardwareRenderer.isWebGL2) return;, the function itself lacks internal protection. A future caller could pass transformFeedbackVaryings without the guard, causing a runtime error. Adding a defensive check (e.g., type guard or early return) would improve safety and prevent potential bugs.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/shader/ShaderProgram.ts` around lines 267 - 274, The
Transform Feedback setup in ShaderProgram currently casts gl to
WebGL2RenderingContext without validation; update the block in the method that
handles transformFeedbackVaryings to first assert that the context is WebGL2
(e.g., using a type guard like "gl instanceof WebGL2RenderingContext" or
checking an existing "isWebGL2" flag) and early-return if it's not WebGL2, then
call (<WebGL2RenderingContext>gl).transformFeedbackVaryings only when that guard
passes to avoid unsafe casts and runtime errors for future callers.

Comment on lines +13 to +17
constructor(rhi: WebGLGraphicDevice) {
const gl = <WebGL2RenderingContext>rhi.gl;
this._gl = gl;
this._glTransformFeedback = gl.createTransformFeedback();
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -e

echo "WebGL2-only transform feedback API usage:"
rg -n -C2 'createTransformFeedback|bindTransformFeedback|beginTransformFeedback|endTransformFeedback' packages/rhi-webgl/src

echo
echo "Creation path into GLTransformFeedback:"
rg -n -C2 'createPlatformTransformFeedback|new GLTransformFeedback|new TransformFeedback\(' packages

Repository: galacean/engine

Length of output: 3528


🏁 Script executed:

# Check WebGLGraphicDevice.createPlatformTransformFeedback and surrounding context
sed -n '265,280p' packages/rhi-webgl/src/WebGLGraphicDevice.ts

# Check TransformFeedback constructor and surrounding context
sed -n '10,25p' packages/core/src/graphic/TransformFeedback.ts

# Check TransformFeedbackPrimitive constructor and surrounding context
sed -n '40,60p' packages/core/src/graphic/TransformFeedbackPrimitive.ts

# Check if isWebGL2 is used elsewhere for feature gating
rg -n 'isWebGL2' packages

Repository: galacean/engine

Length of output: 13563


Guard this constructor against WebGL1 contexts.

rhi.gl can still be a WebGLRenderingContext, and createTransformFeedback() is WebGL2-only. On WebGL1 this becomes a TypeError immediately when instantiated.

The creation path through WebGLGraphicDevice.createPlatformTransformFeedback() and the public TransformFeedback and TransformFeedbackPrimitive constructors have no upstream WebGL2 checks, so the guard must be placed here.

Suggested fix
  constructor(rhi: WebGLGraphicDevice) {
+   if (!rhi.isWebGL2) {
+     throw new Error("Transform Feedback requires a WebGL2 context.");
+   }
    const gl = <WebGL2RenderingContext>rhi.gl;
    this._gl = gl;
    this._glTransformFeedback = gl.createTransformFeedback();
  }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/rhi-webgl/src/GLTransformFeedback.ts` around lines 13 - 17, The
GLTransformFeedback constructor calls createTransformFeedback() on rhi.gl
without ensuring it's a WebGL2 context, causing a TypeError for WebGL1; update
the GLTransformFeedback constructor to check that rhi.gl is an instance of
WebGL2RenderingContext (or that rhi.supportsTransformFeedback/WebGL2 flag is
true) before calling createTransformFeedback(), and if not supported either set
_glTransformFeedback to null/undefined and mark the instance as unsupported or
throw a clear error; make sure callers via
WebGLGraphicDevice.createPlatformTransformFeedback(), TransformFeedback, and
TransformFeedbackPrimitive handle the unsupported case consistently.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (2)
packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts (2)

256-265: ⚠️ Potential issue | 🟠 Major

_isRandomMode() doesn't account for drag's random modes.

The method only checks _limitX, _limitY, _limitZ modes but ignores _drag. When drag uses TwoConstants or TwoCurves while limits use non-random modes, the particle generator won't allocate per-particle random values for drag interpolation, causing all particles to use the same drag factor.

Suggested fix to include drag mode check
   _isRandomMode(): boolean {
+    const dragRandom = this._drag.mode === ParticleCurveMode.TwoConstants || 
+                       this._drag.mode === ParticleCurveMode.TwoCurves;
     if (this._separateAxes) {
       return (
         (this._limitX.mode === ParticleCurveMode.TwoConstants || this._limitX.mode === ParticleCurveMode.TwoCurves) &&
         (this._limitY.mode === ParticleCurveMode.TwoConstants || this._limitY.mode === ParticleCurveMode.TwoCurves) &&
         (this._limitZ.mode === ParticleCurveMode.TwoConstants || this._limitZ.mode === ParticleCurveMode.TwoCurves)
-      );
+      ) || dragRandom;
     }
-    return this._limitX.mode === ParticleCurveMode.TwoConstants || this._limitX.mode === ParticleCurveMode.TwoCurves;
+    return this._limitX.mode === ParticleCurveMode.TwoConstants || 
+           this._limitX.mode === ParticleCurveMode.TwoCurves || 
+           dragRandom;
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts` around
lines 256 - 265, The _isRandomMode() method currently checks only
_limitX/_limitY/_limitZ modes and misses _drag; update _isRandomMode in
LimitVelocityOverLifetimeModule to include a check that _drag.mode is also
TwoConstants or TwoCurves (when deciding random mode), i.e., when _separateAxes
is true require both all three limit axes and _drag be random-mode, and when
_separateAxes is false require either _limitX or _drag to be
TwoConstants/TwoCurves so per-particle random values are allocated for drag
interpolation.

356-421: ⚠️ Potential issue | 🟠 Major

Mixed per-axis modes silently fall back to constant upload.

When separateAxes is enabled with mixed modes (e.g., limitX = Curve, limitY = Constant, limitZ = Curve), lines 368-373 require all axes to be curves, so the code falls through to the constant path (line 402+). This silently discards curve data for axes configured with curves.

Consider either:

  1. Supporting per-axis mode representation in the shader
  2. Rejecting/warning on mixed-mode configurations
  3. Documenting this as expected behavior
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts` around
lines 356 - 421, In _uploadSeparateAxisLimits detect mixed per-axis modes (using
limitX/limitY/limitZ and ParticleCurveMode) and avoid silently falling into the
constant branch: either (A) implement per-axis uploads by setting per-axis
curve/constant shader data (call shaderData.setFloatArray for curve axes using
limitX.curveMax/_curveMin and shaderData.setVector3 for constant axes) and
introduce per-axis mode macros (e.g., add/check new macros like
_limitXCurveMacro/_limitYCurveMacro/_limitZCurveMacro and still set
_limitIsRandomMacro where applicable), or (B) if you prefer not to support mixed
modes, throw/log a clear warning/error from _uploadSeparateAxisLimits when modes
differ instead of uploading constants only; update the function
_uploadSeparateAxisLimits, references to
LimitVelocityOverLifetimeModule._limitCurveModeMacro/_limitConstantModeMacro/_limitIsRandomMacro,
and the shaderData.setFloatArray/setVector3 calls accordingly so curve data is
not discarded silently.
🧹 Nitpick comments (5)
e2e/case/particleRenderer-limitVelocity.ts (1)

136-147: Remove commented-out code.

Lines 136 and 143-146 contain commented-out alternative configurations. These should either be removed or, if they serve as documentation for alternative test scenarios, converted to separate test functions.

Suggested cleanup
   limitVelocityOverLifetime.limitZ = new ParticleCompositeCurve(0);
-  //   limitVelocityOverLifetime.limit = new ParticleCompositeCurve(1);
   limitVelocityOverLifetime.space = ParticleSimulationSpace.World;
   limitVelocityOverLifetime.dampen = 0.25;
   limitVelocityOverLifetime.drag = new ParticleCompositeCurve(0.0);
   limitVelocityOverLifetime.multiplyDragByParticleSize = true;
   limitVelocityOverLifetime.multiplyDragByParticleVelocity = true;
-
-  //   limitVelocityOverLifetime.enabled = true;
-  // limitVelocityOverLifetime.separateAxes = false;
-  // limitVelocityOverLifetime.limit = new ParticleCompositeCurve(0);
-  // limitVelocityOverLifetime.dampen = 1;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@e2e/case/particleRenderer-limitVelocity.ts` around lines 136 - 147, Remove
the commented-out alternative configurations around the
limitVelocityOverLifetime block: delete the commented lines referencing
ParticleCompositeCurve, enabled, separateAxes, and dampen (the comments at and
around the limitVelocityOverLifetime assignment), or if those variants are
useful as documented alternatives, extract them into separate test functions
instead of leaving them commented; locate the limitVelocityOverLifetime usage
and ParticleCompositeCurve / ParticleSimulationSpace references in this file to
remove or refactor the commented alternatives accordingly.
packages/core/src/graphic/TransformFeedbackPrimitive.ts (2)

154-160: Consider reusing VertexBufferBinding objects to reduce allocations.

swap() creates a new VertexBufferBinding instance on every call. Since TF updates happen every frame for active particle systems, this causes per-frame allocations. Consider maintaining two pre-allocated binding objects and swapping between them.

Suggested optimization
+  private _renderBufferBindingA: VertexBufferBinding;
+  private _renderBufferBindingB: VertexBufferBinding;
+  private _useBindingA = true;

   resize(vertexCount: number): void {
     // ... existing code ...
     this._readBuffer = readBuffer;
     this._writeBuffer = writeBuffer;
-    this._renderBufferBinding = new VertexBufferBinding(this._readBuffer, this._byteStride);
+    this._renderBufferBindingA = new VertexBufferBinding(readBuffer, this._byteStride);
+    this._renderBufferBindingB = new VertexBufferBinding(writeBuffer, this._byteStride);
+    this._renderBufferBinding = this._renderBufferBindingA;
+    this._useBindingA = true;
     // ...
   }

   swap(): void {
     const temp = this._readBuffer;
     this._readBuffer = this._writeBuffer;
     this._writeBuffer = temp;
     this._currentVAO = this._currentVAO === this._vaoA ? this._vaoB : this._vaoA;
-    this._renderBufferBinding = new VertexBufferBinding(this._readBuffer, this._byteStride);
+    this._useBindingA = !this._useBindingA;
+    this._renderBufferBinding = this._useBindingA ? this._renderBufferBindingA : this._renderBufferBindingB;
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/graphic/TransformFeedbackPrimitive.ts` around lines 154 -
160, The swap() method in TransformFeedbackPrimitive currently allocates a new
VertexBufferBinding each call (this._renderBufferBinding = new
VertexBufferBinding(...)); instead, pre-allocate two VertexBufferBinding
instances (e.g., this._bindingA / this._bindingB) as members alongside
_vaoA/_vaoB and initialize them for the corresponding buffers and _byteStride,
then in swap() simply toggle which binding is active (assign
this._renderBufferBinding = this._bindingA or this._bindingB) and update their
internal buffer reference/stride if necessary; update any initialization code
that creates _renderBufferBinding to use the pre-allocated pair and ensure
_renderBufferBinding is swapped when _readBuffer/_writeBuffer and _currentVAO
are swapped.

61-86: Consider resetting _currentVAO reference in resize() to avoid stale reference.

After resize() sets _vaoDirty = true, the old VAOs will be deleted in rebuildVAOsIfNeeded(). However, if bindVAO() were called before rebuildVAOsIfNeeded(), _currentVAO would reference a deleted VAO. While the current usage pattern appears safe (rebuild is always called before bind), explicitly nullifying _currentVAO in resize() would make the code more defensive.

Suggested defensive fix
     this._vertexCount = vertexCount;
     this._initialized = true;
     this._vaoDirty = true;
+    this._currentVAO = null;
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/graphic/TransformFeedbackPrimitive.ts` around lines 61 -
86, The resize() method should clear the cached VAO reference to avoid holding a
pointer to a VAO that will be destroyed; add a line to set this._currentVAO =
null (or undefined consistent with class) when resizing—ideally right after
setting this._vaoDirty = true—so that rebuildVAOsIfNeeded() can safely recreate
VAOs and bindVAO() cannot reference a deleted object.
packages/core/src/particle/ParticleTransformFeedbackSimulator.ts (2)

127-127: Replace magic number with GL constant for clarity.

POINTS = 0x0000 is the raw WebGL constant value. Consider using gl.POINTS for better readability and maintainability.

Suggested fix
+    const gl = this._engine._hardwareRenderer.gl;
     // Bind VAO and execute TF for alive particles
     this._primitive.bindVAO();

-    const POINTS = 0x0000;
     if (firstActive < firstFree) {
-      this._primitive.draw(rhi, POINTS, firstActive, firstFree - firstActive);
+      this._primitive.draw(rhi, gl.POINTS, firstActive, firstFree - firstActive);
     } else {
-      this._primitive.draw(rhi, POINTS, firstActive, particleCount - firstActive);
+      this._primitive.draw(rhi, gl.POINTS, firstActive, particleCount - firstActive);
       if (firstFree > 0) {
-        this._primitive.draw(rhi, 0, firstFree);
+        this._primitive.draw(rhi, gl.POINTS, 0, firstFree);
       }
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts` at line
127, Replace the magic numeric literal used for POINTS with the WebGL constant
from the existing GL context: change the declaration that sets POINTS = 0x0000
to use the in-scope WebGLRenderingContext constant (e.g., POINTS = gl.POINTS or
this.gl.POINTS depending on how the context is referenced in
ParticleTransformFeedbackSimulator) so the code uses the named constant instead
of the raw hex value.

109-113: Per-frame VertexBufferBinding allocation in update loop.

VertexBufferBinding is instantiated on every update() call (line 110). Since this runs every frame for active particle systems, it creates unnecessary GC pressure. Consider caching and reusing the binding when the instance buffer hasn't changed.

Suggested optimization
+  private _cachedInstanceBuffer: Buffer;
+  private _cachedInstanceBinding: VertexBufferBinding;

   update(
     instanceBuffer: Buffer,
     // ...
   ): void {
     // ...
-    const instanceBinding = new VertexBufferBinding(instanceBuffer, ParticleBufferUtils.instanceVertexStride);
+    if (this._cachedInstanceBuffer !== instanceBuffer) {
+      this._cachedInstanceBuffer = instanceBuffer;
+      this._cachedInstanceBinding = new VertexBufferBinding(instanceBuffer, ParticleBufferUtils.instanceVertexStride);
+    }
+    const instanceBinding = this._cachedInstanceBinding;
     this._primitive.rebuildVAOsIfNeeded(this._tfProgram, ParticleTransformFeedbackSimulator._tfElements, [
       { binding: instanceBinding, elements: ParticleTransformFeedbackSimulator._instanceElements }
     ]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts` around
lines 109 - 113, The code currently allocates a new VertexBufferBinding each
frame inside update(), causing GC churn; cache and reuse the binding instead:
add a class field (e.g., this._instanceBinding) and in update() only create a
new VertexBufferBinding when the instanceBuffer reference changes (compare
instanceBuffer to the cached binding's buffer or null), otherwise reuse the
cached binding when calling this._primitive.rebuildVAOsIfNeeded(this._tfProgram,
ParticleTransformFeedbackSimulator._tfElements, [{ binding:
this._instanceBinding, elements:
ParticleTransformFeedbackSimulator._instanceElements }]); ensure the cache is
cleared/updated whenever the instanceBuffer is replaced or disposed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts`:
- Around line 256-265: The _isRandomMode() method currently checks only
_limitX/_limitY/_limitZ modes and misses _drag; update _isRandomMode in
LimitVelocityOverLifetimeModule to include a check that _drag.mode is also
TwoConstants or TwoCurves (when deciding random mode), i.e., when _separateAxes
is true require both all three limit axes and _drag be random-mode, and when
_separateAxes is false require either _limitX or _drag to be
TwoConstants/TwoCurves so per-particle random values are allocated for drag
interpolation.
- Around line 356-421: In _uploadSeparateAxisLimits detect mixed per-axis modes
(using limitX/limitY/limitZ and ParticleCurveMode) and avoid silently falling
into the constant branch: either (A) implement per-axis uploads by setting
per-axis curve/constant shader data (call shaderData.setFloatArray for curve
axes using limitX.curveMax/_curveMin and shaderData.setVector3 for constant
axes) and introduce per-axis mode macros (e.g., add/check new macros like
_limitXCurveMacro/_limitYCurveMacro/_limitZCurveMacro and still set
_limitIsRandomMacro where applicable), or (B) if you prefer not to support mixed
modes, throw/log a clear warning/error from _uploadSeparateAxisLimits when modes
differ instead of uploading constants only; update the function
_uploadSeparateAxisLimits, references to
LimitVelocityOverLifetimeModule._limitCurveModeMacro/_limitConstantModeMacro/_limitIsRandomMacro,
and the shaderData.setFloatArray/setVector3 calls accordingly so curve data is
not discarded silently.

---

Nitpick comments:
In `@e2e/case/particleRenderer-limitVelocity.ts`:
- Around line 136-147: Remove the commented-out alternative configurations
around the limitVelocityOverLifetime block: delete the commented lines
referencing ParticleCompositeCurve, enabled, separateAxes, and dampen (the
comments at and around the limitVelocityOverLifetime assignment), or if those
variants are useful as documented alternatives, extract them into separate test
functions instead of leaving them commented; locate the
limitVelocityOverLifetime usage and ParticleCompositeCurve /
ParticleSimulationSpace references in this file to remove or refactor the
commented alternatives accordingly.

In `@packages/core/src/graphic/TransformFeedbackPrimitive.ts`:
- Around line 154-160: The swap() method in TransformFeedbackPrimitive currently
allocates a new VertexBufferBinding each call (this._renderBufferBinding = new
VertexBufferBinding(...)); instead, pre-allocate two VertexBufferBinding
instances (e.g., this._bindingA / this._bindingB) as members alongside
_vaoA/_vaoB and initialize them for the corresponding buffers and _byteStride,
then in swap() simply toggle which binding is active (assign
this._renderBufferBinding = this._bindingA or this._bindingB) and update their
internal buffer reference/stride if necessary; update any initialization code
that creates _renderBufferBinding to use the pre-allocated pair and ensure
_renderBufferBinding is swapped when _readBuffer/_writeBuffer and _currentVAO
are swapped.
- Around line 61-86: The resize() method should clear the cached VAO reference
to avoid holding a pointer to a VAO that will be destroyed; add a line to set
this._currentVAO = null (or undefined consistent with class) when
resizing—ideally right after setting this._vaoDirty = true—so that
rebuildVAOsIfNeeded() can safely recreate VAOs and bindVAO() cannot reference a
deleted object.

In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts`:
- Line 127: Replace the magic numeric literal used for POINTS with the WebGL
constant from the existing GL context: change the declaration that sets POINTS =
0x0000 to use the in-scope WebGLRenderingContext constant (e.g., POINTS =
gl.POINTS or this.gl.POINTS depending on how the context is referenced in
ParticleTransformFeedbackSimulator) so the code uses the named constant instead
of the raw hex value.
- Around line 109-113: The code currently allocates a new VertexBufferBinding
each frame inside update(), causing GC churn; cache and reuse the binding
instead: add a class field (e.g., this._instanceBinding) and in update() only
create a new VertexBufferBinding when the instanceBuffer reference changes
(compare instanceBuffer to the cached binding's buffer or null), otherwise reuse
the cached binding when calling
this._primitive.rebuildVAOsIfNeeded(this._tfProgram,
ParticleTransformFeedbackSimulator._tfElements, [{ binding:
this._instanceBinding, elements:
ParticleTransformFeedbackSimulator._instanceElements }]); ensure the cache is
cleared/updated whenever the instanceBuffer is replaced or disposed.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: d59df521-c2b6-4d48-b48b-0b1f6cd258e6

📥 Commits

Reviewing files that changed from the base of the PR and between 2193eb7 and 3c88bf0.

⛔ Files ignored due to path filters (1)
  • e2e/fixtures/originImage/Particle_particleRenderer-limitVelocity.jpg is excluded by !**/*.jpg
📒 Files selected for processing (6)
  • e2e/case/particleRenderer-limitVelocity.ts
  • e2e/config.ts
  • packages/core/src/graphic/TransformFeedbackPrimitive.ts
  • packages/core/src/particle/ParticleRenderer.ts
  • packages/core/src/particle/ParticleTransformFeedbackSimulator.ts
  • packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • e2e/config.ts

- Fix animatedVelocity composition: only VOL, not FOL/gravity
- Fix gravity timing: add to base velocity before dampen/drag
- Fix drag math: scale total then subtract animated (not scale base only)
- Remove Unity references from comments

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts (1)

44-44: Inconsistent shader property naming.

_dragConstantProperty uses "u_DragConstant" while all other properties in this module use the "renderer_LVL*" prefix (e.g., renderer_LVLDampen, renderer_LVLDragMaxCurve). Consider renaming to "renderer_LVLDragConstant" for consistency.

Suggested fix
-  static readonly _dragConstantProperty = ShaderProperty.getByName("u_DragConstant");
+  static readonly _dragConstantProperty = ShaderProperty.getByName("renderer_LVLDragConstant");
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts` at
line 44, Update the shader property name for consistency: change the string
passed to ShaderProperty.getByName in the static member _dragConstantProperty
from "u_DragConstant" to "renderer_LVLDragConstant" and ensure any other usages
that reference the shader property by name align with this new identifier (look
for references to _dragConstantProperty and any direct "u_DragConstant" usages
to update them).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts`:
- Around line 423-445: In _uploadDrag, constant-based drag modes never set a
"random" macro so the shader can't tell TwoConstants (random) from Constant;
update _uploadDrag to set and return a dedicated macro (e.g.,
LimitVelocityOverLifetimeModule._dragIsRandomMacro) when drag.mode ===
ParticleCurveMode.TwoConstants (in addition to writing _dragConstantProperty
using _dragConstantVec), keep existing behavior of returning _dragCurveModeMacro
for curve modes, and ensure the shader code uses that new _dragIsRandomMacro
(and related symbol _isRandomMode()) to choose interpolation; reference symbols:
_uploadDrag, _drag, _dragConstantVec, _dragConstantProperty,
_dragCurveModeMacro, _dragMin/Max curve properties, and add/return
LimitVelocityOverLifetimeModule._dragIsRandomMacro when appropriate.

In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts`:
- Around line 64-83: The code marks the transform-feedback simulator initialized
even when resized to zero, allowing writeParticleData to write into
zero-capacity buffers; fix by only setting this->_initialized (in resize) when
particleCount > 0 (or set it false when particleCount === 0), and/or add a
defensive check at the start of writeParticleData to return if the primitive
buffers have insufficient capacity (use
ParticleTransformFeedbackSimulator._byteStride and the index to compute/validate
byteOffset before calling _primitive.readBuffer.setData and
_primitive.writeBuffer.setData) so writes never occur into zero-sized ping-pong
buffers.

---

Nitpick comments:
In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts`:
- Line 44: Update the shader property name for consistency: change the string
passed to ShaderProperty.getByName in the static member _dragConstantProperty
from "u_DragConstant" to "renderer_LVLDragConstant" and ensure any other usages
that reference the shader property by name align with this new identifier (look
for references to _dragConstantProperty and any direct "u_DragConstant" usages
to update them).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 2703be57-bb7a-4843-8e3a-adad99afc684

📥 Commits

Reviewing files that changed from the base of the PR and between 3c88bf0 and d1b3bc2.

⛔ Files ignored due to path filters (1)
  • packages/core/src/shaderlib/particle/particle_transform_feedback_update.glsl is excluded by !**/*.glsl
📒 Files selected for processing (3)
  • packages/core/src/particle/ParticleRenderer.ts
  • packages/core/src/particle/ParticleTransformFeedbackSimulator.ts
  • packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts

Comment on lines +423 to +445
private _uploadDrag(shaderData: ShaderData): ShaderMacro {
const drag = this._drag;
let dragCurveMacro: ShaderMacro = null;

const isRandomCurveMode = drag.mode === ParticleCurveMode.TwoCurves;
if (isRandomCurveMode || drag.mode === ParticleCurveMode.Curve) {
shaderData.setFloatArray(LimitVelocityOverLifetimeModule._dragMaxCurveProperty, drag.curveMax._getTypeArray());
dragCurveMacro = LimitVelocityOverLifetimeModule._dragCurveModeMacro;
if (isRandomCurveMode) {
shaderData.setFloatArray(LimitVelocityOverLifetimeModule._dragMinCurveProperty, drag.curveMin._getTypeArray());
}
} else {
const dragVec = this._dragConstantVec;
if (drag.mode === ParticleCurveMode.TwoConstants) {
dragVec.set(drag.constantMin, drag.constantMax);
} else {
dragVec.set(drag.constantMax, drag.constantMax);
}
shaderData.setVector2(LimitVelocityOverLifetimeModule._dragConstantProperty, dragVec);
}

return dragCurveMacro;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Drag constant modes lack a random indicator macro.

For constant-based drag modes (TwoConstants vs Constant), no macro is returned to inform the shader whether to interpolate between min and max using a random value. The shader receives a Vector2 but cannot distinguish between "use both with random" and "use single value".

This is related to the _isRandomMode() issue—without proper random mode detection and a dedicated macro (similar to _limitIsRandomMacro), drag randomization may not work as expected.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts` around
lines 423 - 445, In _uploadDrag, constant-based drag modes never set a "random"
macro so the shader can't tell TwoConstants (random) from Constant; update
_uploadDrag to set and return a dedicated macro (e.g.,
LimitVelocityOverLifetimeModule._dragIsRandomMacro) when drag.mode ===
ParticleCurveMode.TwoConstants (in addition to writing _dragConstantProperty
using _dragConstantVec), keep existing behavior of returning _dragCurveModeMacro
for curve modes, and ensure the shader code uses that new _dragIsRandomMacro
(and related symbol _isRandomMode()) to choose interpolation; reference symbols:
_uploadDrag, _drag, _dragConstantVec, _dragConstantProperty,
_dragCurveModeMacro, _dragMin/Max curve properties, and add/return
LimitVelocityOverLifetimeModule._dragIsRandomMacro when appropriate.

GuoLei1990 and others added 4 commits March 17, 2026 16:22
- Unbind TF buffer from TRANSFORM_FEEDBACK_BUFFER target after TF draw
  to prevent conflict when render VAO rebuilds (e.g., VOL enabled at runtime)
- Fix drag math: scale total velocity then subtract animated
- Fix gravity: add to base velocity before dampen/drag
- Fix animatedVelocity: only VOL, not FOL/gravity

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace raw GL constants with MeshTopology.Points
- Reorder TransformFeedback methods by usage flow
- Add @param annotations to TransformFeedback API

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move VAO management to GLTransformFeedbackPrimitive (RHI layer)
- Add IPlatformTransformFeedbackPrimitive interface
- Remove GL calls from core layer TransformFeedbackPrimitive
- Split draw into beginDraw/draw/endDraw (no array allocation)
- Use VertexElement/VertexBufferBinding directly (no intermediate types)
- Remove unused drawArrays/VAO methods from WebGLGraphicDevice
- Fix TF buffer residual binding causing GL conflict on VAO rebuild

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
GuoLei1990 and others added 9 commits March 17, 2026 17:33
…point

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Simplify to readBinding/writeBinding (remove separate buffer fields)
- Rename update to updateVertexLayout
- Split beginDraw/draw/endDraw for flexible ring buffer drawing
- Move render binding cache into primitive (zero GC swap)
- Clean up comments to use core-layer concepts
- Move VAO logic to GLTransformFeedbackPrimitive via platform interface
- Remove unused drawArrays/VAO methods from WebGLGraphicDevice

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove section separator comments from static fields
- Simplify limit property comment
- Clamp dampen to [0, 1] matching Unity behavior

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…sistency

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…uffers

- Core layer no longer accesses _platformBuffer
- RHI layer extracts platform buffer internally (same pattern as GLPrimitive)
- Fix cross-package TS error for Buffer._platformBuffer

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TF object internally remembers buffer bindings. When bind() restores
the TF object, old buffer binding resurfaces on TRANSFORM_FEEDBACK_BUFFER,
conflicting with the same buffer on ARRAY_BUFFER via VAO.
Clear with unbindBuffer(0) after bind() before setting new range.
Also add invalidate() to force VAO rebuild after buffer resize.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pass _bindingA/_bindingB directly to updateVertexLayout instead of
readBinding/writeBinding which vary with swap state. This prevents
VAO A from accidentally binding buffer B (and vice versa) when
program recompiles after an odd number of swaps.

Also remove redundant unbindBuffer in beginDraw.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
VOL is animated velocity (reset each frame), should use the full
instantaneous value, not delta between frames. Constant VOL had
zero delta, making it invisible. Now matches Unity's behavior where
animatedVelocity is reset to zero then VOL adds full value.

Also fix VAO A/B correspondence to always match binding A/B.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
GuoLei1990 and others added 8 commits March 17, 2026 23:12
…form upload

- TF pass now runs in ParticleRenderer._update after shader data is ready
- Remove redundant _updateShaderData and _currentTimeProperty from Generator
- Extract _updateFeedback method for clean separation
- Simplify condition checks (remove redundant _feedbackSimulator null checks)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Single curve drag mode was unconditionally mixing with uninitialized
MinCurve (defaulting to zero), causing drag to be randomly weakened.
Now only TwoCurves mode does min/max interpolation; single curve
returns dragMax directly.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use ParticleFeedbackVertexAttribute enum for vertex element names
- Simplify _setTransformFeedback (remove redundant guards and locals)
- Remove unused Logger and ShaderProperty imports
- Remove redundant _feedbackBindingIndex >= 0 checks
- Clean up comments

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
limitVal → limitValue, minLimitVal → minLimitValue,
ConstVec → ConstVector for uniform naming consistency.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use Vector3.transformByQuat instead of hand-written quaternion rotation
- Accept Vector3 position in writeParticleData
- Skip copy in local mode (pass shapePosition directly)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Deduplicate upload logic using compact flag for bufferByteOffset
- Remove trailing periods from comments

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ack_simulation

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

♻️ Duplicate comments (4)
packages/rhi-webgl/src/GLTransformFeedbackPrimitive.ts (1)

27-35: ⚠️ Potential issue | 🟠 Major

Rebuild VAOs when buffer bindings change, not only when program changes.

Line 27 currently keys cache on program.id only. If readBinding, writeBinding, or inputBinding points to a new buffer with the same program, VAOs keep stale bindings.

Suggested fix
 export class GLTransformFeedbackPrimitive implements IPlatformTransformFeedbackPrimitive {
   private _gl: WebGL2RenderingContext;
   private _vaoA: WebGLVertexArrayObject;
   private _vaoB: WebGLVertexArrayObject;
   private _lastProgramId = -1;
+  private _lastReadBuffer: unknown = null;
+  private _lastWriteBuffer: unknown = null;
+  private _lastInputBuffer: unknown = null;
@@
   updateVertexLayout(
     program: any,
     readBinding: VertexBufferBinding,
     writeBinding: VertexBufferBinding,
@@
   ): void {
-    if (program.id === this._lastProgramId) return;
+    const readBuffer = readBinding?.buffer;
+    const writeBuffer = writeBinding?.buffer;
+    const inputBuffer = inputBinding?.buffer;
+    if (
+      program.id === this._lastProgramId &&
+      readBuffer === this._lastReadBuffer &&
+      writeBuffer === this._lastWriteBuffer &&
+      inputBuffer === this._lastInputBuffer
+    ) {
+      return;
+    }
@@
     this._vaoA = this._createVAO(attribs, readBinding, feedbackElements, inputBinding, inputElements);
     this._vaoB = this._createVAO(attribs, writeBinding, feedbackElements, inputBinding, inputElements);
     this._lastProgramId = program.id;
+    this._lastReadBuffer = readBuffer;
+    this._lastWriteBuffer = writeBuffer;
+    this._lastInputBuffer = inputBuffer;
@@
   private _deleteVAOs(): void {
@@
     this._lastProgramId = -1;
+    this._lastReadBuffer = null;
+    this._lastWriteBuffer = null;
+    this._lastInputBuffer = null;
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/rhi-webgl/src/GLTransformFeedbackPrimitive.ts` around lines 27 - 35,
The code only rebuilds VAOs when program.id changes (checked via
this._lastProgramId), so VAOs stay stale if readBinding, writeBinding, or
inputBinding change; update the logic to also detect binding changes by tracking
unique identifiers for readBinding, writeBinding and inputBinding (e.g., buffer
id or a composite key) and include them in the cache check before returning, and
when any of those ids differ call this._deleteVAOs() and recreate
this._vaoA/this._vaoB via this._createVAO(attribs, ...), then update the stored
last-program and last-binding ids (e.g., this._lastReadBindingId,
this._lastWriteBindingId, this._lastInputBindingId) alongside
this._lastProgramId.
packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts (1)

260-269: ⚠️ Potential issue | 🟠 Major

Include drag random modes in module random-mode detection.

Line 260-Line 269 currently ignores drag random modes. This can skip per-particle random generation when drag is TwoConstants/TwoCurves but limits are non-random.

Suggested fix
   _isRandomMode(): boolean {
+    const dragRandom =
+      this._drag.mode === ParticleCurveMode.TwoConstants || this._drag.mode === ParticleCurveMode.TwoCurves;
+
+    const limitRandom = this._separateAxes
+      ? (this._limitX.mode === ParticleCurveMode.TwoConstants || this._limitX.mode === ParticleCurveMode.TwoCurves) &&
+        (this._limitY.mode === ParticleCurveMode.TwoConstants || this._limitY.mode === ParticleCurveMode.TwoCurves) &&
+        (this._limitZ.mode === ParticleCurveMode.TwoConstants || this._limitZ.mode === ParticleCurveMode.TwoCurves)
+      : this._limitX.mode === ParticleCurveMode.TwoConstants || this._limitX.mode === ParticleCurveMode.TwoCurves;
+
-    if (this._separateAxes) {
-      return (
-        (this._limitX.mode === ParticleCurveMode.TwoConstants || this._limitX.mode === ParticleCurveMode.TwoCurves) &&
-        (this._limitY.mode === ParticleCurveMode.TwoConstants || this._limitY.mode === ParticleCurveMode.TwoCurves) &&
-        (this._limitZ.mode === ParticleCurveMode.TwoConstants || this._limitZ.mode === ParticleCurveMode.TwoCurves)
-      );
-    }
-    return this._limitX.mode === ParticleCurveMode.TwoConstants || this._limitX.mode === ParticleCurveMode.TwoCurves;
+    return limitRandom || dragRandom;
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts` around
lines 260 - 269, The _isRandomMode method currently only checks limit curves and
ignores drag random modes; update it to also consider drag being in random modes
(ParticleCurveMode.TwoConstants or ParticleCurveMode.TwoCurves). Specifically,
in the branch where this._separateAxes is true, include checks for
this._dragX.mode, this._dragY.mode and this._dragZ.mode (alongside
_limitX/_limitY/_limitZ) and in the non-separate branch include this._drag.mode
in the OR logic with this._limitX.mode so that any drag TwoConstants/TwoCurves
causes _isRandomMode to return true.
packages/rhi-webgl/src/GLTransformFeedback.ts (1)

13-17: ⚠️ Potential issue | 🔴 Critical

Add explicit WebGL2 and handle-creation guards in constructor.

This constructor still assumes WebGL2 and does not check createTransformFeedback() result, which can fail and produce invalid state.

Suggested patch
  constructor(rhi: WebGLGraphicDevice) {
+    if (!rhi.isWebGL2) {
+      throw new Error("Transform Feedback requires a WebGL2 context.");
+    }
     const gl = <WebGL2RenderingContext>rhi.gl;
     this._gl = gl;
-    this._glTransformFeedback = gl.createTransformFeedback();
+    const tf = gl.createTransformFeedback();
+    if (!tf) {
+      throw new Error("Failed to create WebGLTransformFeedback.");
+    }
+    this._glTransformFeedback = tf;
   }
#!/bin/bash
set -e
# Confirm guard presence and constructor call paths
sed -n '1,120p' packages/rhi-webgl/src/GLTransformFeedback.ts
rg -nP --type=ts 'new GLTransformFeedback\s*\(' packages
rg -nP --type=ts -C3 'createPlatformTransformFeedback|isWebGL2' packages/rhi-webgl/src
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/rhi-webgl/src/GLTransformFeedback.ts` around lines 13 - 17, The
GLTransformFeedback constructor assumes WebGL2 and doesn't validate
createTransformFeedback() output; update the constructor in class
GLTransformFeedback to assert/guard that rhi.gl is a WebGL2RenderingContext (use
isWebGL2 or instanceof WebGL2RenderingContext) and check the result of
gl.createTransformFeedback(); if it returns null handle it by throwing or
returning a clear error (or set a safe fallback and log) and ensure
_glTransformFeedback is only assigned when non-null, referencing the
constructor, WebGL2RenderingContext, createTransformFeedback, and
_glTransformFeedback symbols.
e2e/case/particleRenderer-limitVelocity.ts (1)

27-28: ⚠️ Potential issue | 🟡 Minor

Wire the screenshot harness in this case setup.

initScreenshot/updateForE2E are imported but unused; the case never explicitly settles before capture. This can make snapshots timing-sensitive.

Suggested patch
-    .then((texture) => {
-      createScalarLimitParticle(engine, rootEntity, <Texture2D>texture);
+    .then((texture) => {
+      createScalarLimitParticle(engine, rootEntity, camera, <Texture2D>texture);
     });
 });
 
-function createScalarLimitParticle(engine: Engine, rootEntity: Entity, texture: Texture2D): void {
+function createScalarLimitParticle(engine: Engine, rootEntity: Entity, camera: Camera, texture: Texture2D): void {
@@
   limitVelocityOverLifetime.drag = new ParticleCompositeCurve(0.0);
   limitVelocityOverLifetime.multiplyDragByParticleSize = true;
   limitVelocityOverLifetime.multiplyDragByParticleVelocity = true;
+
+  updateForE2E(engine, 500);
+  initScreenshot(engine, camera);
 }

Also applies to: 64-68, 148-148

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@e2e/case/particleRenderer-limitVelocity.ts` around lines 27 - 28, The test
imports initScreenshot and updateForE2E but never uses them, so wire the
screenshot harness into the case: call initScreenshot() near the start of the
test setup to initialize the harness and, after any actions that change state
but before taking a snapshot, call await updateForE2E() to let the scene settle;
update the particleRenderer-limitVelocity test case (references to
initScreenshot and updateForE2E in the file) to include these calls in the setup
and immediately before captures so snapshots are not timing-sensitive.
🧹 Nitpick comments (2)
packages/core/src/particle/ParticleTransformFeedbackSimulator.ts (1)

84-84: Avoid per-frame VertexBufferBinding allocation in hot update loop.

Line 84 allocates every frame. Reusing a cached binding (or mutating an existing one) will reduce GC churn during particle-heavy scenes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts` at line 84,
The per-frame allocation of a VertexBufferBinding (variable instanceBinding)
inside ParticleTransformFeedbackSimulator causes GC churn; cache a reusable
binding on the simulator instance instead of constructing it each update: add a
class field (e.g., this.instanceBinding) created once using instanceBuffer and
ParticleBufferUtils.instanceVertexStride and reuse it every frame, updating its
buffer reference only if instanceBuffer changes (or mutate the existing binding)
so you stop allocating a new VertexBufferBinding each frame.
packages/core/src/shader/ShaderPass.ts (1)

153-163: Consider centralizing capability macro collection.

_getShaderLabProgram() rebuilds the same renderer-capability macro list that the non-ShaderLab path already needs. Pulling that into a shared helper would reduce drift if new capability macros get added later.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/shader/ShaderPass.ts` around lines 153 - 163, The code in
_getShaderLabProgram rebuilds renderer capability macros (e.g.,
GRAPHICS_API_WEBGL2/1, HAS_TEX_LOD, HAS_DERIVATIVES) into shaderMacroList;
extract that logic into a shared helper (for example a new private function like
collectRendererCapabilityMacros(engine: Engine, target: ShaderMacro[])) and call
it from _getShaderLabProgram and the non-ShaderLab path so both use the same
single source of truth; ensure the helper uses engine._hardwareRenderer and
ShaderMacro.getByName to push the same macros and update callers to remove the
duplicated push logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@e2e/case/particleRenderer-limitVelocity.ts`:
- Around line 123-129: Remove the leftover debug logging inside the setTimeout
callback: locate the anonymous callback that sets velocityOverLifetime.enabled
and its velocityX/Y/Z constants and delete the console.log("s") call so the E2E
path no longer emits debug output; ensure only the velocityOverLifetime changes
remain in that callback.

In `@packages/core/src/particle/ParticleGenerator.ts`:
- Around line 987-1002: The initial velocity is not rotated into world space
when this.main.simulationSpace !== ParticleSimulationSpace.Local, so rotate the
local direction by transform.worldRotationQuaternion before passing to
writeParticleData: after computing world position (using
ParticleGenerator._tempVector32 and transform.worldRotationQuaternion),
transform the local direction vector (the existing direction variable) by
transform.worldRotationQuaternion into a temp vector (e.g., a ParticleGenerator
temp Vector3), then use that rotated vector multiplied by startSpeed for the
x/y/z velocity args in this._feedbackSimulator.writeParticleData so
world-simulated particles respect emitter rotation.

In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts`:
- Around line 85-105: beginUpdate()/endUpdate() pair in
ParticleTransformFeedbackSimulator is not exception-safe: if any
this._simulator.draw(...) throws, endUpdate() is skipped; wrap the draw logic in
a try/finally so endUpdate() always executes when beginUpdate(...) returned
true. Concretely, after the successful beginUpdate(...) call on this._simulator,
enclose the conditional draw(...) calls (including both branches using
MeshTopology.Points and the two draw calls) inside a try block and call
this._simulator.endUpdate() in the finally block; preserve the early return when
beginUpdate(...) returns false so endUpdate() is only invoked when
beginUpdate(...) succeeded.

In `@packages/rhi-webgl/src/WebGLGraphicDevice.ts`:
- Around line 279-281: createPlatformTransformFeedbackPrimitive() assumes WebGL2
and will crash on WebGL1; add a guard using this._isWebGL2 before instantiating
GLTransformFeedbackPrimitive. In createPlatformTransformFeedbackPrimitive(),
check this._isWebGL2 and if true return new
GLTransformFeedbackPrimitive(<WebGL2RenderingContext>this._gl); otherwise either
throw a clear Error (e.g. "Transform feedback requires WebGL2") or return a
no-op/fallback IPlatformTransformFeedbackPrimitive implementation to match how
other WebGL2-only methods are protected.

---

Duplicate comments:
In `@e2e/case/particleRenderer-limitVelocity.ts`:
- Around line 27-28: The test imports initScreenshot and updateForE2E but never
uses them, so wire the screenshot harness into the case: call initScreenshot()
near the start of the test setup to initialize the harness and, after any
actions that change state but before taking a snapshot, call await
updateForE2E() to let the scene settle; update the
particleRenderer-limitVelocity test case (references to initScreenshot and
updateForE2E in the file) to include these calls in the setup and immediately
before captures so snapshots are not timing-sensitive.

In `@packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts`:
- Around line 260-269: The _isRandomMode method currently only checks limit
curves and ignores drag random modes; update it to also consider drag being in
random modes (ParticleCurveMode.TwoConstants or ParticleCurveMode.TwoCurves).
Specifically, in the branch where this._separateAxes is true, include checks for
this._dragX.mode, this._dragY.mode and this._dragZ.mode (alongside
_limitX/_limitY/_limitZ) and in the non-separate branch include this._drag.mode
in the OR logic with this._limitX.mode so that any drag TwoConstants/TwoCurves
causes _isRandomMode to return true.

In `@packages/rhi-webgl/src/GLTransformFeedback.ts`:
- Around line 13-17: The GLTransformFeedback constructor assumes WebGL2 and
doesn't validate createTransformFeedback() output; update the constructor in
class GLTransformFeedback to assert/guard that rhi.gl is a
WebGL2RenderingContext (use isWebGL2 or instanceof WebGL2RenderingContext) and
check the result of gl.createTransformFeedback(); if it returns null handle it
by throwing or returning a clear error (or set a safe fallback and log) and
ensure _glTransformFeedback is only assigned when non-null, referencing the
constructor, WebGL2RenderingContext, createTransformFeedback, and
_glTransformFeedback symbols.

In `@packages/rhi-webgl/src/GLTransformFeedbackPrimitive.ts`:
- Around line 27-35: The code only rebuilds VAOs when program.id changes
(checked via this._lastProgramId), so VAOs stay stale if readBinding,
writeBinding, or inputBinding change; update the logic to also detect binding
changes by tracking unique identifiers for readBinding, writeBinding and
inputBinding (e.g., buffer id or a composite key) and include them in the cache
check before returning, and when any of those ids differ call this._deleteVAOs()
and recreate this._vaoA/this._vaoB via this._createVAO(attribs, ...), then
update the stored last-program and last-binding ids (e.g.,
this._lastReadBindingId, this._lastWriteBindingId, this._lastInputBindingId)
alongside this._lastProgramId.

---

Nitpick comments:
In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts`:
- Line 84: The per-frame allocation of a VertexBufferBinding (variable
instanceBinding) inside ParticleTransformFeedbackSimulator causes GC churn;
cache a reusable binding on the simulator instance instead of constructing it
each update: add a class field (e.g., this.instanceBinding) created once using
instanceBuffer and ParticleBufferUtils.instanceVertexStride and reuse it every
frame, updating its buffer reference only if instanceBuffer changes (or mutate
the existing binding) so you stop allocating a new VertexBufferBinding each
frame.

In `@packages/core/src/shader/ShaderPass.ts`:
- Around line 153-163: The code in _getShaderLabProgram rebuilds renderer
capability macros (e.g., GRAPHICS_API_WEBGL2/1, HAS_TEX_LOD, HAS_DERIVATIVES)
into shaderMacroList; extract that logic into a shared helper (for example a new
private function like collectRendererCapabilityMacros(engine: Engine, target:
ShaderMacro[])) and call it from _getShaderLabProgram and the non-ShaderLab path
so both use the same single source of truth; ensure the helper uses
engine._hardwareRenderer and ShaderMacro.getByName to push the same macros and
update callers to remove the duplicated push logic.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 4fc14c71-df4e-45c3-86f7-9d341f40ef7b

📥 Commits

Reviewing files that changed from the base of the PR and between 3c88bf0 and 63fd990.

⛔ Files ignored due to path filters (3)
  • packages/core/src/shaderlib/extra/particle.vs.glsl is excluded by !**/*.glsl
  • packages/core/src/shaderlib/particle/limit_velocity_over_lifetime_module.glsl is excluded by !**/*.glsl
  • packages/core/src/shaderlib/particle/particle_feedback_simulation.glsl is excluded by !**/*.glsl
📒 Files selected for processing (19)
  • e2e/case/particleRenderer-limitVelocity.ts
  • packages/core/src/graphic/TransformFeedback.ts
  • packages/core/src/graphic/TransformFeedbackPrimitive.ts
  • packages/core/src/graphic/TransformFeedbackSimulator.ts
  • packages/core/src/particle/ParticleBufferUtils.ts
  • packages/core/src/particle/ParticleGenerator.ts
  • packages/core/src/particle/ParticleRenderer.ts
  • packages/core/src/particle/ParticleTransformFeedbackSimulator.ts
  • packages/core/src/particle/enums/attributes/ParticleFeedbackVertexAttribute.ts
  • packages/core/src/particle/modules/LimitVelocityOverLifetimeModule.ts
  • packages/core/src/renderingHardwareInterface/IPlatformTransformFeedback.ts
  • packages/core/src/renderingHardwareInterface/IPlatformTransformFeedbackPrimitive.ts
  • packages/core/src/renderingHardwareInterface/index.ts
  • packages/core/src/shader/ShaderPass.ts
  • packages/core/src/shaderlib/ShaderFactory.ts
  • packages/core/src/shaderlib/particle/index.ts
  • packages/rhi-webgl/src/GLTransformFeedback.ts
  • packages/rhi-webgl/src/GLTransformFeedbackPrimitive.ts
  • packages/rhi-webgl/src/WebGLGraphicDevice.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/core/src/renderingHardwareInterface/IPlatformTransformFeedback.ts
  • packages/core/src/graphic/TransformFeedback.ts

Comment on lines +987 to +1002
let position: Vector3;
if (this.main.simulationSpace === ParticleSimulationSpace.Local) {
position = shapePosition;
} else {
position = ParticleGenerator._tempVector32;
Vector3.transformByQuat(shapePosition, transform.worldRotationQuaternion, position);
position.add(transform.worldPosition);
}

this._feedbackSimulator.writeParticleData(
index,
position,
direction.x * startSpeed,
direction.y * startSpeed,
direction.z * startSpeed
);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Rotate initial TF velocity in world simulation mode.

In world mode, position is rotated into world space (Lines 991–994), but velocity at Lines 999–1001 still uses local direction. This causes incorrect initial motion when emitter rotation is non-identity.

Suggested patch
   private _addFeedbackParticle(
@@
   ): void {
     let position: Vector3;
+    let velocity = direction;
     if (this.main.simulationSpace === ParticleSimulationSpace.Local) {
       position = shapePosition;
     } else {
       position = ParticleGenerator._tempVector32;
       Vector3.transformByQuat(shapePosition, transform.worldRotationQuaternion, position);
       position.add(transform.worldPosition);
+      velocity = ParticleGenerator._tempVector31;
+      Vector3.transformByQuat(direction, transform.worldRotationQuaternion, velocity);
     }
 
     this._feedbackSimulator.writeParticleData(
       index,
       position,
-      direction.x * startSpeed,
-      direction.y * startSpeed,
-      direction.z * startSpeed
+      velocity.x * startSpeed,
+      velocity.y * startSpeed,
+      velocity.z * startSpeed
     );
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/particle/ParticleGenerator.ts` around lines 987 - 1002, The
initial velocity is not rotated into world space when this.main.simulationSpace
!== ParticleSimulationSpace.Local, so rotate the local direction by
transform.worldRotationQuaternion before passing to writeParticleData: after
computing world position (using ParticleGenerator._tempVector32 and
transform.worldRotationQuaternion), transform the local direction vector (the
existing direction variable) by transform.worldRotationQuaternion into a temp
vector (e.g., a ParticleGenerator temp Vector3), then use that rotated vector
multiplied by startSpeed for the x/y/z velocity args in
this._feedbackSimulator.writeParticleData so world-simulated particles respect
emitter rotation.

Comment on lines +85 to +105
if (
!this._simulator.beginUpdate(
shaderData,
ParticleBufferUtils.feedbackVertexElements,
instanceBinding,
ParticleBufferUtils.feedbackInstanceElements
)
)
return;

if (firstActive < firstFree) {
this._simulator.draw(MeshTopology.Points, firstActive, firstFree - firstActive);
} else {
this._simulator.draw(MeshTopology.Points, firstActive, particleCount - firstActive);
if (firstFree > 0) {
this._simulator.draw(MeshTopology.Points, 0, firstFree);
}
}

this._simulator.endUpdate();
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Make TF update exception-safe with try/finally.

If any draw-path call throws after beginUpdate(), endUpdate() is skipped and TF state cleanup is missed.

Suggested patch
     if (
       !this._simulator.beginUpdate(
         shaderData,
         ParticleBufferUtils.feedbackVertexElements,
         instanceBinding,
         ParticleBufferUtils.feedbackInstanceElements
       )
     )
       return;
 
-    if (firstActive < firstFree) {
-      this._simulator.draw(MeshTopology.Points, firstActive, firstFree - firstActive);
-    } else {
-      this._simulator.draw(MeshTopology.Points, firstActive, particleCount - firstActive);
-      if (firstFree > 0) {
-        this._simulator.draw(MeshTopology.Points, 0, firstFree);
+    try {
+      if (firstActive < firstFree) {
+        this._simulator.draw(MeshTopology.Points, firstActive, firstFree - firstActive);
+      } else {
+        this._simulator.draw(MeshTopology.Points, firstActive, particleCount - firstActive);
+        if (firstFree > 0) {
+          this._simulator.draw(MeshTopology.Points, 0, firstFree);
+        }
       }
+    } finally {
+      this._simulator.endUpdate();
     }
-
-    this._simulator.endUpdate();
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts` around
lines 85 - 105, beginUpdate()/endUpdate() pair in
ParticleTransformFeedbackSimulator is not exception-safe: if any
this._simulator.draw(...) throws, endUpdate() is skipped; wrap the draw logic in
a try/finally so endUpdate() always executes when beginUpdate(...) returned
true. Concretely, after the successful beginUpdate(...) call on this._simulator,
enclose the conditional draw(...) calls (including both branches using
MeshTopology.Points and the two draw calls) inside a try block and call
this._simulator.endUpdate() in the finally block; preserve the early return when
beginUpdate(...) returns false so endUpdate() is only invoked when
beginUpdate(...) succeeded.

Comment on lines +279 to +281
createPlatformTransformFeedbackPrimitive(): IPlatformTransformFeedbackPrimitive {
return new GLTransformFeedbackPrimitive(<WebGL2RenderingContext>this._gl);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -e
# Verify whether this factory can be reached without explicit WebGL2 gating
rg -nP --type=ts '\bcreatePlatformTransformFeedbackPrimitive\s*\(' packages
rg -nP --type=ts -C3 'new GLTransformFeedbackPrimitive|isWebGL2' packages/rhi-webgl/src
sed -n '1,140p' packages/rhi-webgl/src/GLTransformFeedbackPrimitive.ts

Repository: galacean/engine

Length of output: 37600


Guard TF primitive creation behind WebGL2 capability.

createPlatformTransformFeedbackPrimitive() currently assumes a WebGL2 context without checking. In a WebGL1 runtime, this will fail at first API call. Other methods in the same class (e.g., enableRasterizerDiscard, blitFrameBuffer) properly guard WebGL2-only operations using this._isWebGL2.

Suggested patch
  createPlatformTransformFeedbackPrimitive(): IPlatformTransformFeedbackPrimitive {
+    if (!this._isWebGL2) {
+      throw new Error("Transform Feedback primitive requires a WebGL2 context.");
+    }
    return new GLTransformFeedbackPrimitive(<WebGL2RenderingContext>this._gl);
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
createPlatformTransformFeedbackPrimitive(): IPlatformTransformFeedbackPrimitive {
return new GLTransformFeedbackPrimitive(<WebGL2RenderingContext>this._gl);
}
createPlatformTransformFeedbackPrimitive(): IPlatformTransformFeedbackPrimitive {
if (!this._isWebGL2) {
throw new Error("Transform Feedback primitive requires a WebGL2 context.");
}
return new GLTransformFeedbackPrimitive(<WebGL2RenderingContext>this._gl);
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/rhi-webgl/src/WebGLGraphicDevice.ts` around lines 279 - 281,
createPlatformTransformFeedbackPrimitive() assumes WebGL2 and will crash on
WebGL1; add a guard using this._isWebGL2 before instantiating
GLTransformFeedbackPrimitive. In createPlatformTransformFeedbackPrimitive(),
check this._isWebGL2 and if true return new
GLTransformFeedbackPrimitive(<WebGL2RenderingContext>this._gl); otherwise either
throw a clear Error (e.g. "Transform feedback requires WebGL2") or return a
no-op/fallback IPlatformTransformFeedbackPrimitive implementation to match how
other WebGL2-only methods are protected.

GuoLei1990 and others added 2 commits March 18, 2026 00:11
- Create entity before adding to scene to prevent early play()
- Use updateForE2E + initScreenshot instead of engine.run()
- Remove PostProcess/Bloom and cleanup unused code

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update origin images for forceOverLifetime, limitVelocityOverLifetime,
and particleEmitMeshCone.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@e2e/case/particleRenderer-limitVelocity.ts`:
- Around line 122-123: Before enabling limitVelocityOverLifetime, add an
explicit WebGL2 capability guard that checks the renderer/context for WebGL2
(e.g. renderer.capabilities.isWebGL2 or gl instanceof WebGL2RenderingContext)
and fail or skip the test if WebGL2 is not available; specifically, insert the
guard just before the lines that set limitVelocityOverLifetime.enabled and
.separateAxes so the test does not silently no-op on WebGL1—use a clear early
exit such as throwing an Error("WebGL2 required for limit-velocity test") or
invoking the test runner's skip mechanism.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 8fb17f70-130d-494e-add2-dc0b53b550af

📥 Commits

Reviewing files that changed from the base of the PR and between 63fd990 and 24d4600.

📒 Files selected for processing (1)
  • e2e/case/particleRenderer-limitVelocity.ts

Comment on lines +122 to +123
limitVelocityOverLifetime.enabled = true;
limitVelocityOverLifetime.separateAxes = true;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Verify module behavior: enabling is gated by WebGL2.
module_file="$(fd 'LimitVelocityOverLifetimeModule.ts$' | head -n1)"
case_file="$(fd 'particleRenderer-limitVelocity.ts$' | head -n1)"

echo "Module file: ${module_file}"
echo "Case file: ${case_file}"

rg -n -C3 'override set enabled|isWebGL2|_setTransformFeedback' "$module_file"
rg -n -C3 'limitVelocityOverLifetime\.enabled|isWebGL2' "$case_file"

# Optional: inspect similar E2E patterns for capability gating.
rg -n --type=ts -C2 'isWebGL2|TransformFeedback|limitVelocityOverLifetime' e2e/case

Repository: galacean/engine

Length of output: 2685


Fail fast when WebGL2 is unavailable so this test actually exercises limit-velocity.

The module's enabled setter silently returns without enabling on WebGL1 (it checks isWebGL2 and returns early). Add an explicit WebGL2 capability guard before line 122 to ensure the test fails or skips if WebGL2 is not available, rather than silently passing without covering the intended transform-feedback behavior.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@e2e/case/particleRenderer-limitVelocity.ts` around lines 122 - 123, Before
enabling limitVelocityOverLifetime, add an explicit WebGL2 capability guard that
checks the renderer/context for WebGL2 (e.g. renderer.capabilities.isWebGL2 or
gl instanceof WebGL2RenderingContext) and fail or skip the test if WebGL2 is not
available; specifically, insert the guard just before the lines that set
limitVelocityOverLifetime.enabled and .separateAxes so the test does not
silently no-op on WebGL1—use a clear early exit such as throwing an
Error("WebGL2 required for limit-velocity test") or invoking the test runner's
skip mechanism.

GuoLei1990 and others added 2 commits March 18, 2026 00:43
…ibute

a_Random2 was conditionally declared using _FOL_MODULE_ENABLED, but
this macro is defined inside force_over_lifetime_module.glsl which
was included after the declaration. Move declaration after includes
so the macro is available when the condition is evaluated.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
GLSL requires declaration before use. a_Random2 must be declared
before force_over_lifetime_module.glsl is included (which uses it).
Use RENDERER_FOL_CONSTANT_MODE/RENDERER_FOL_CURVE_MODE (CPU macros)
instead of _FOL_MODULE_ENABLED (defined inside the include).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pass existing VertexBufferBinding from ParticleGenerator on resize,
eliminating per-frame allocation in update().

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (3)
packages/core/src/particle/ParticleGenerator.ts (2)

979-1001: ⚠️ Potential issue | 🟠 Major

Rotate initial TF velocity for world simulation.

In world mode, Line 991-992 rotates/emits position in world space, but Line 998-1000 still uses local direction. This produces incorrect initial motion when emitter rotation is non-identity.

Suggested patch
   private _addFeedbackParticle(
     index: number,
     shapePosition: Vector3,
     direction: Vector3,
     startSpeed: number,
     transform: Transform
   ): void {
     let position: Vector3;
+    let velocity: Vector3 = direction;
     if (this.main.simulationSpace === ParticleSimulationSpace.Local) {
       position = shapePosition;
     } else {
       position = ParticleGenerator._tempVector32;
       Vector3.transformByQuat(shapePosition, transform.worldRotationQuaternion, position);
       position.add(transform.worldPosition);
+      velocity = ParticleGenerator._tempVector31;
+      Vector3.transformByQuat(direction, transform.worldRotationQuaternion, velocity);
     }
 
     this._feedbackSimulator.writeParticleData(
       index,
       position,
-      direction.x * startSpeed,
-      direction.y * startSpeed,
-      direction.z * startSpeed
+      velocity.x * startSpeed,
+      velocity.y * startSpeed,
+      velocity.z * startSpeed
     );
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/particle/ParticleGenerator.ts` around lines 979 - 1001, The
feedback particle's initial velocity isn't rotated into world space in
ParticleGenerator._addFeedbackParticle when this.main.simulationSpace !==
ParticleSimulationSpace.Local; keep the existing world-space position transform
but also rotate the local direction by transform.worldRotationQuaternion (e.g.,
into a temporary Vector3) and use that rotated direction's x/y/z multiplied by
startSpeed in the call to this._feedbackSimulator.writeParticleData so emitted
velocities respect emitter rotation.

614-628: ⚠️ Potential issue | 🟠 Major

Backfill alive particles when enabling TF at runtime.

Line 621 only resizes TF buffers. Existing alive slots are not copied into TF state, so the first TF step can read default position/velocity for already-active particles.

Suggested patch sketch
   _setTransformFeedback(enabled: boolean): void {
     this._useTransformFeedback = enabled;
 
     if (enabled) {
       if (!this._feedbackSimulator) {
         this._feedbackSimulator = new ParticleTransformFeedbackSimulator(this._renderer.engine);
       }
       this._feedbackSimulator.resize(this._currentParticleCount, this._instanceVertexBufferBinding);
+      this._syncAliveParticlesToFeedback();
       this._renderer.shaderData.enableMacro(ParticleGenerator._transformFeedbackMacro);
     } else {
       this._renderer.shaderData.disableMacro(ParticleGenerator._transformFeedbackMacro);
     }
 
     this._reorganizeGeometryBuffers();
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/particle/ParticleGenerator.ts` around lines 614 - 628, When
enabling transform-feedback in _setTransformFeedback, after creating/resizing
this._feedbackSimulator call a method to initialize/copy the current alive
particle state from the instance vertex data into the TF buffers so the first TF
step doesn't read default values; use this._instanceVertexBufferBinding and
this._currentParticleCount as the source and add/invoke a method on
ParticleTransformFeedbackSimulator (e.g. initializeFromInstanceBinding or
copyFromInstanceBuffers) to transfer per-particle attributes before enabling the
macro and before calling _reorganizeGeometryBuffers().
packages/core/src/particle/ParticleTransformFeedbackSimulator.ts (1)

84-104: ⚠️ Potential issue | 🟠 Major

Guard TF state cleanup with try/finally.

Line 85 enters TF update, but if any draw on Line 95/Line 97/Line 99 throws, Line 103 is skipped and TF state is left unclosed.

Suggested patch
     if (
       !this._simulator.beginUpdate(
         shaderData,
         ParticleBufferUtils.feedbackVertexElements,
         this._instanceBinding,
         ParticleBufferUtils.feedbackInstanceElements
       )
     )
       return;
 
-    if (firstActive < firstFree) {
-      this._simulator.draw(MeshTopology.Points, firstActive, firstFree - firstActive);
-    } else {
-      this._simulator.draw(MeshTopology.Points, firstActive, particleCount - firstActive);
-      if (firstFree > 0) {
-        this._simulator.draw(MeshTopology.Points, 0, firstFree);
+    try {
+      if (firstActive < firstFree) {
+        this._simulator.draw(MeshTopology.Points, firstActive, firstFree - firstActive);
+      } else {
+        this._simulator.draw(MeshTopology.Points, firstActive, particleCount - firstActive);
+        if (firstFree > 0) {
+          this._simulator.draw(MeshTopology.Points, 0, firstFree);
+        }
       }
+    } finally {
+      this._simulator.endUpdate();
     }
-
-    this._simulator.endUpdate();
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts` around
lines 84 - 104, The TF (transform feedback) update calls into this._simulator
beginUpdate/draw/endUpdate must be wrapped so endUpdate always runs even if a
draw throws: capture the boolean result of this._simulator.beginUpdate(...) into
a local (e.g., began), and if began then execute the draw logic inside a try
block and call this._simulator.endUpdate() in a finally block; do not call
endUpdate when beginUpdate returned false. Update the code around
_simulator.beginUpdate / _simulator.draw / _simulator.endUpdate to use this
try/finally pattern so TF state is always cleaned up.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@packages/core/src/particle/ParticleGenerator.ts`:
- Around line 979-1001: The feedback particle's initial velocity isn't rotated
into world space in ParticleGenerator._addFeedbackParticle when
this.main.simulationSpace !== ParticleSimulationSpace.Local; keep the existing
world-space position transform but also rotate the local direction by
transform.worldRotationQuaternion (e.g., into a temporary Vector3) and use that
rotated direction's x/y/z multiplied by startSpeed in the call to
this._feedbackSimulator.writeParticleData so emitted velocities respect emitter
rotation.
- Around line 614-628: When enabling transform-feedback in
_setTransformFeedback, after creating/resizing this._feedbackSimulator call a
method to initialize/copy the current alive particle state from the instance
vertex data into the TF buffers so the first TF step doesn't read default
values; use this._instanceVertexBufferBinding and this._currentParticleCount as
the source and add/invoke a method on ParticleTransformFeedbackSimulator (e.g.
initializeFromInstanceBinding or copyFromInstanceBuffers) to transfer
per-particle attributes before enabling the macro and before calling
_reorganizeGeometryBuffers().

In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts`:
- Around line 84-104: The TF (transform feedback) update calls into
this._simulator beginUpdate/draw/endUpdate must be wrapped so endUpdate always
runs even if a draw throws: capture the boolean result of
this._simulator.beginUpdate(...) into a local (e.g., began), and if began then
execute the draw logic inside a try block and call this._simulator.endUpdate()
in a finally block; do not call endUpdate when beginUpdate returned false.
Update the code around _simulator.beginUpdate / _simulator.draw /
_simulator.endUpdate to use this try/finally pattern so TF state is always
cleaned up.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: f9965229-ef8a-46c0-9911-ab36dbc8e709

📥 Commits

Reviewing files that changed from the base of the PR and between dc064eb and 429db08.

⛔ Files ignored due to path filters (2)
  • e2e/fixtures/originImage/Particle_particleRenderer-force.jpg is excluded by !**/*.jpg
  • packages/core/src/shaderlib/extra/particle.vs.glsl is excluded by !**/*.glsl
📒 Files selected for processing (2)
  • packages/core/src/particle/ParticleGenerator.ts
  • packages/core/src/particle/ParticleTransformFeedbackSimulator.ts

@hhhhkrx hhhhkrx requested a review from zhuxudong March 18, 2026 02:56
a_Random0.x is the gravity modifier random channel, not an independent
drag factor. This caused drag TwoConstants/TwoCurves to be coupled with
gravity config. Now shares a_Random2.w with LVL limit random factor.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
feedbackVaryings: string[]
) {
this._engine = engine;
this._primitive = new TransformFeedbackPrimitive(engine, byteStride);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 这条 Transform Feedback 路径还没有真正接进设备恢复链路。TransformFeedbackSimulator 自己维护了一套独立的 ShaderProgramPool,不会随着 devicerestored 一起清空;恢复后 beginUpdate() 可能继续复用底层 GL program 已失效的 ShaderProgram 包装对象,而 GLTransformFeedbackPrimitive.updateVertexLayout() 又只在 program.id 变化时才重建 VAO。另外,feedback ping-pong buffer 恢复后是空的,但粒子路径只会重传 instance buffer,不会恢复已存活粒子的当前位置/速度。既然引擎显式支持 context lost/restore,这条 TF 模拟路径需要明确的 invalidate/恢复策略。

singlecoder and others added 4 commits March 18, 2026 14:01
* fix: compontont props clone
Added Buffer.copyFromBuffer (backed by gl.copyBufferSubData) so
TransformFeedbackPrimitive.resize migrates old data on the GPU
before destroying the previous buffers.  This fixes the bright dot
at the emitter origin on first play when a burst triggers buffer
expansion before the first TF simulate pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Mar 18, 2026
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
packages/ui/src/component/interactive/UIInteractive.ts (1)

21-22: ⚠️ Potential issue | 🔴 Critical

Mark _rootCanvas and _group with @ignoreClone decorator to prevent reference to original objects on clone.

These fields lack @ignoreClone decorators and will be cloned by reference. Since UICanvas and UIGroup do not define _remap methods, the cloned component will retain references to the original canvas and group. Additionally, the corresponding dirty flags (_isRootCanvasDirty, _isGroupDirty) are marked with @ignoreClone and initialize to false, preventing automatic re-discovery via Utils.searchRootCanvasInParents and Utils.searchGroupInParents in the getter methods. Add @ignoreClone to these field declarations so they're rediscovered on the cloned instance.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/component/interactive/UIInteractive.ts` around lines 21 - 22,
In UIInteractive, mark the fields _rootCanvas and _group with the `@ignoreClone`
decorator (matching the existing `@ignoreClone` on _isRootCanvasDirty and
_isGroupDirty) so cloned instances do not retain references to the original
UICanvas/UIGroup; this ensures the boolean flags (_isRootCanvasDirty,
_isGroupDirty) remain false and the getters will trigger re-discovery via
Utils.searchRootCanvasInParents and Utils.searchGroupInParents instead of
pointing to the original objects.
packages/ui/src/component/UICanvas.ts (1)

48-49: ⚠️ Potential issue | 🟠 Major

Restore @ignoreClone on _cameraObserver field.

The _cameraObserver field is derived state calculated from _camera, _isRootCanvas, and _renderMode. Removing @ignoreClone causes it to be remapped as a component reference during cloning. This breaks state consistency: if the source canvas's renderMode is the default WorldSpace and remains WorldSpace after cloning, the _updateCameraObserver() method is never invoked (setter only triggers on change). The cloned canvas retains a reference to the remapped camera without registering the necessary event listeners (_onCameraTransformListener and _onCameraModifyListener), creating stale state that can cause incorrect listener management later.

Also applies to: lines 81-82

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/component/UICanvas.ts` around lines 48 - 49, Re-add the
`@ignoreClone` annotation to the _cameraObserver field so it is not treated as a
remapped component reference during cloning; this ensures cloned canvases keep
derived state and trigger _updateCameraObserver() properly instead of inheriting
a remapped camera reference without listeners. Specifically, mark the
_cameraObserver declaration with `@ignoreClone` and do the same for the related
field referenced at lines 81-82 (also a derived/internal field) so cloning does
not remap those component references; keep _camera, _isRootCanvas and
_renderMode unchanged and rely on _updateCameraObserver() to register
_onCameraTransformListener and _onCameraModifyListener after clone
initialization.
♻️ Duplicate comments (2)
packages/core/src/particle/ParticleTransformFeedbackSimulator.ts (1)

84-104: ⚠️ Potential issue | 🟠 Major

Make TF update exception-safe with try/finally.

If any draw() call throws after beginUpdate() succeeds, endUpdate() is skipped. This leaves GL state corrupted (rasterizer discard enabled, transform feedback bound), which would break subsequent rendering.

Suggested fix
     if (
       !this._simulator.beginUpdate(
         shaderData,
         ParticleBufferUtils.feedbackVertexElements,
         this._instanceBinding,
         ParticleBufferUtils.feedbackInstanceElements
       )
     )
       return;
 
-    if (firstActive < firstFree) {
-      this._simulator.draw(MeshTopology.Points, firstActive, firstFree - firstActive);
-    } else {
-      this._simulator.draw(MeshTopology.Points, firstActive, particleCount - firstActive);
-      if (firstFree > 0) {
-        this._simulator.draw(MeshTopology.Points, 0, firstFree);
+    try {
+      if (firstActive < firstFree) {
+        this._simulator.draw(MeshTopology.Points, firstActive, firstFree - firstActive);
+      } else {
+        this._simulator.draw(MeshTopology.Points, firstActive, particleCount - firstActive);
+        if (firstFree > 0) {
+          this._simulator.draw(MeshTopology.Points, 0, firstFree);
+        }
       }
+    } finally {
+      this._simulator.endUpdate();
     }
-
-    this._simulator.endUpdate();
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts` around
lines 84 - 104, The code calls this._simulator.beginUpdate(...), then performs
one or more this._simulator.draw(...) calls and finally
this._simulator.endUpdate(); make the update exception-safe by wrapping the draw
logic in a try/finally so that this._simulator.endUpdate() is always invoked
even if any draw() throws; locate the beginUpdate / draw / endUpdate calls in
ParticleTransformFeedbackSimulator and move the draw branches into the try block
with endUpdate() in the finally block to guarantee GL state is cleaned up.
packages/core/src/particle/ParticleGenerator.ts (1)

978-1001: ⚠️ Potential issue | 🟠 Major

Rotate initial TF velocity in world simulation mode.

Position is correctly transformed to world space (Lines 989-991), but velocity still uses local-space direction. This causes incorrect initial motion when the emitter has non-identity rotation.

Suggested fix
   private _addFeedbackParticle(
     index: number,
     shapePosition: Vector3,
     direction: Vector3,
     startSpeed: number,
     transform: Transform
   ): void {
     let position: Vector3;
+    let velocity = direction;
     if (this.main.simulationSpace === ParticleSimulationSpace.Local) {
       position = shapePosition;
     } else {
       position = ParticleGenerator._tempVector32;
       Vector3.transformByQuat(shapePosition, transform.worldRotationQuaternion, position);
       position.add(transform.worldPosition);
+      velocity = ParticleGenerator._tempVector31;
+      Vector3.transformByQuat(direction, transform.worldRotationQuaternion, velocity);
     }
 
     this._feedbackSimulator.writeParticleData(
       index,
       position,
-      direction.x * startSpeed,
-      direction.y * startSpeed,
-      direction.z * startSpeed
+      velocity.x * startSpeed,
+      velocity.y * startSpeed,
+      velocity.z * startSpeed
     );
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/particle/ParticleGenerator.ts` around lines 978 - 1001, The
initial velocity isn't rotated into world space in
ParticleGenerator._addFeedbackParticle when this.main.simulationSpace !==
ParticleSimulationSpace.Local; rotate the local-space direction by
transform.worldRotationQuaternion before scaling by startSpeed and passing
components to this._feedbackSimulator.writeParticleData. Use a temp vector
(don't overwrite shapePosition temp used for position—e.g., a dedicated temp
like ParticleGenerator._tempVector33 or a new local Vector3), call
Vector3.transformByQuat(direction, transform.worldRotationQuaternion,
rotatedDir), multiply rotatedDir by startSpeed, then pass rotatedDir.x/y/z to
writeParticleData instead of direction.x/y/z.
🧹 Nitpick comments (1)
packages/core/src/Transform.ts (1)

54-54: Verify intentional removal of @ignoreClone from _parentTransformCache.

The @ignoreClone decorator was removed from _parentTransformCache. While _isParentDirty (line 53) is still @ignoreClone and defaults to true, ensuring the cache is recalculated on first access, the removal allows the cached parent reference to be processed during cloning.

Since Transform extends Component and has _remap, the reference should be correctly remapped if the parent is within the cloned subtree. However, this adds unnecessary cloning work since _isParentDirty = true will force recalculation anyway.

Consider keeping @ignoreClone on _parentTransformCache to avoid unnecessary remapping overhead:

+ `@ignoreClone`
  private _parentTransformCache: Transform = null;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/Transform.ts` at line 54, The _parentTransformCache field
lost its `@ignoreClone` decorator allowing it to be processed during cloning and
causing unnecessary remap work even though _isParentDirty (still `@ignoreClone`)
will force recompute; restore the `@ignoreClone` decorator on the
_parentTransformCache property in the Transform class so the cached reference is
skipped by the clone machinery (keeping _isParentDirty true ensures
recalculation) and mention/verify that _remap on Component still correctly
handles cases where the parent is inside the cloned subtree.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/core/src/clone/CloneManager.ts`:
- Around line 140-159: The array branch fails to register the source array in
deepInstanceMap and reuses the parent cloneMode for every element; before
iterating set deepInstanceMap.set(sourceProperty, targetPropertyA) to
register/reuse shared or self-referential arrays, and compute an
element-specific clone mode rather than reusing the parent cloneMode when
calling CloneManager.cloneProperty (e.g., derive elementCloneMode from the
array's own clone metadata or a dedicated helper and pass that to
CloneManager.cloneProperty instead of cloneMode).
- Around line 175-181: The recursive clone call in CloneManager is not
propagating a parent Deep setting into undecorated nested fields, so in
CloneManager.cloneProperty (called from the loop using
sourceProperty/targetProperty and cloneModes[k]) update the logic to derive an
effective mode: if the parent field's mode (from
CloneManager.getCloneMode(sourceProperty.constructor) or the caller's current
mode) is CloneMode.Deep and cloneModes[k] is undefined, treat the effective mode
as CloneMode.Deep before calling CloneManager.cloneProperty; ensure the
effectiveMode (not just cloneModes[k]) is passed down into recursive calls so
nested plain objects/arrays inherit Deep when their parent was annotated.
- Around line 161-188: The code currently continues to recurse even when
deepInstanceMap already contains sourceProperty, causing duplicate traversal and
infinite recursion on cycles; modify the block in CloneManager.cloneProperty so
that after fetching targetProperty = deepInstanceMap.get(sourceProperty) and
assigning target[k] = targetProperty you immediately return (or otherwise skip
the subsequent copy/iteration logic) instead of proceeding to call copyFrom,
iterate properties, or invoke _cloneTo; this ensures when deepInstanceMap has
sourceProperty you reuse the mapped target instance and do not walk the same
source object again.

In `@packages/math/src/CollisionUtil.ts`:
- Around line 148-156: The JSDoc for intersectsRayAndPlane (in CollisionUtil.ts)
and the Ray return description (in Ray.ts) must be updated to state that when a
ray is parallel to the plane but its origin lies on the plane the function
returns 0 (intersection at origin) instead of -1; update the comments to
explicitly describe the parallel-on-plane edge case and that -1 is returned only
when there is no intersection. Also verify downstream behavior in XRHitTest.ts
(the if (distance >= 0) check) and UIRenderer.ts (the if (curDistance >= 0 &&
curDistance < distance) check) to ensure treating parallel-on-plane as a valid
hit at distance 0 is intended—either leave as-is if intended or adjust those
conditional checks to exclude parallel-origin cases, and add/update
unit/integration tests to cover this scenario.

In `@packages/ui/src/component/advanced/Text.ts`:
- Around line 270-275: Remove the `@assignmentClone` decorator from the _font
field and mark it `@ignoreClone` like _subFont, then rely on the existing override
_cloneTo(target: Text) to assign via target.font = this._font so the font setter
runs and reference counting occurs; specifically update the _font declaration to
use `@ignoreClone` (remove `@assignmentClone`) and keep the _cloneTo method as-is so
the font setter's guard (in the font property/method) increments/decrements refs
correctly during cloning.

---

Outside diff comments:
In `@packages/ui/src/component/interactive/UIInteractive.ts`:
- Around line 21-22: In UIInteractive, mark the fields _rootCanvas and _group
with the `@ignoreClone` decorator (matching the existing `@ignoreClone` on
_isRootCanvasDirty and _isGroupDirty) so cloned instances do not retain
references to the original UICanvas/UIGroup; this ensures the boolean flags
(_isRootCanvasDirty, _isGroupDirty) remain false and the getters will trigger
re-discovery via Utils.searchRootCanvasInParents and Utils.searchGroupInParents
instead of pointing to the original objects.

In `@packages/ui/src/component/UICanvas.ts`:
- Around line 48-49: Re-add the `@ignoreClone` annotation to the _cameraObserver
field so it is not treated as a remapped component reference during cloning;
this ensures cloned canvases keep derived state and trigger
_updateCameraObserver() properly instead of inheriting a remapped camera
reference without listeners. Specifically, mark the _cameraObserver declaration
with `@ignoreClone` and do the same for the related field referenced at lines
81-82 (also a derived/internal field) so cloning does not remap those component
references; keep _camera, _isRootCanvas and _renderMode unchanged and rely on
_updateCameraObserver() to register _onCameraTransformListener and
_onCameraModifyListener after clone initialization.

---

Duplicate comments:
In `@packages/core/src/particle/ParticleGenerator.ts`:
- Around line 978-1001: The initial velocity isn't rotated into world space in
ParticleGenerator._addFeedbackParticle when this.main.simulationSpace !==
ParticleSimulationSpace.Local; rotate the local-space direction by
transform.worldRotationQuaternion before scaling by startSpeed and passing
components to this._feedbackSimulator.writeParticleData. Use a temp vector
(don't overwrite shapePosition temp used for position—e.g., a dedicated temp
like ParticleGenerator._tempVector33 or a new local Vector3), call
Vector3.transformByQuat(direction, transform.worldRotationQuaternion,
rotatedDir), multiply rotatedDir by startSpeed, then pass rotatedDir.x/y/z to
writeParticleData instead of direction.x/y/z.

In `@packages/core/src/particle/ParticleTransformFeedbackSimulator.ts`:
- Around line 84-104: The code calls this._simulator.beginUpdate(...), then
performs one or more this._simulator.draw(...) calls and finally
this._simulator.endUpdate(); make the update exception-safe by wrapping the draw
logic in a try/finally so that this._simulator.endUpdate() is always invoked
even if any draw() throws; locate the beginUpdate / draw / endUpdate calls in
ParticleTransformFeedbackSimulator and move the draw branches into the try block
with endUpdate() in the finally block to guarantee GL state is cleaned up.

---

Nitpick comments:
In `@packages/core/src/Transform.ts`:
- Line 54: The _parentTransformCache field lost its `@ignoreClone` decorator
allowing it to be processed during cloning and causing unnecessary remap work
even though _isParentDirty (still `@ignoreClone`) will force recompute; restore
the `@ignoreClone` decorator on the _parentTransformCache property in the
Transform class so the cached reference is skipped by the clone machinery
(keeping _isParentDirty true ensures recalculation) and mention/verify that
_remap on Component still correctly handles cases where the parent is inside the
cloned subtree.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5b210b67-5995-421c-8173-24d724de3a3b

📥 Commits

Reviewing files that changed from the base of the PR and between 429db08 and 47d64aa.

⛔ Files ignored due to path filters (1)
  • packages/core/src/shaderlib/particle/particle_feedback_simulation.glsl is excluded by !**/*.glsl
📒 Files selected for processing (51)
  • e2e/package.json
  • examples/package.json
  • package.json
  • packages/core/package.json
  • packages/core/src/2d/sprite/SpriteMask.ts
  • packages/core/src/2d/sprite/SpriteRenderer.ts
  • packages/core/src/2d/text/TextRenderer.ts
  • packages/core/src/Camera.ts
  • packages/core/src/Component.ts
  • packages/core/src/Entity.ts
  • packages/core/src/Renderer.ts
  • packages/core/src/Transform.ts
  • packages/core/src/animation/Animator.ts
  • packages/core/src/audio/AudioSource.ts
  • packages/core/src/clone/CloneManager.ts
  • packages/core/src/clone/CloneUtils.ts
  • packages/core/src/clone/ComponentCloner.ts
  • packages/core/src/graphic/Buffer.ts
  • packages/core/src/graphic/TransformFeedbackPrimitive.ts
  • packages/core/src/mesh/MeshRenderer.ts
  • packages/core/src/mesh/Skin.ts
  • packages/core/src/mesh/SkinnedMeshRenderer.ts
  • packages/core/src/particle/ParticleGenerator.ts
  • packages/core/src/particle/ParticleTransformFeedbackSimulator.ts
  • packages/core/src/particle/modules/shape/MeshShape.ts
  • packages/core/src/physics/shape/ColliderShape.ts
  • packages/core/src/renderingHardwareInterface/IPlatformBuffer.ts
  • packages/design/package.json
  • packages/galacean/package.json
  • packages/loader/package.json
  • packages/math/package.json
  • packages/math/src/CollisionUtil.ts
  • packages/physics-lite/package.json
  • packages/physics-physx/package.json
  • packages/rhi-webgl/package.json
  • packages/rhi-webgl/src/GLBuffer.ts
  • packages/shader-lab/package.json
  • packages/shader/package.json
  • packages/ui/package.json
  • packages/ui/src/component/UICanvas.ts
  • packages/ui/src/component/UIGroup.ts
  • packages/ui/src/component/UIRenderer.ts
  • packages/ui/src/component/UITransform.ts
  • packages/ui/src/component/advanced/Button.ts
  • packages/ui/src/component/advanced/Image.ts
  • packages/ui/src/component/advanced/Text.ts
  • packages/ui/src/component/interactive/UIInteractive.ts
  • packages/xr-webxr/package.json
  • packages/xr/package.json
  • tests/package.json
  • tests/src/core/CloneUtils.test.ts
💤 Files with no reviewable changes (3)
  • packages/core/src/physics/shape/ColliderShape.ts
  • packages/ui/src/component/UIRenderer.ts
  • packages/ui/src/component/UIGroup.ts
✅ Files skipped from review due to trivial changes (14)
  • packages/shader-lab/package.json
  • packages/shader/package.json
  • examples/package.json
  • packages/physics-physx/package.json
  • packages/xr/package.json
  • packages/loader/package.json
  • tests/package.json
  • e2e/package.json
  • packages/physics-lite/package.json
  • packages/design/package.json
  • packages/galacean/package.json
  • package.json
  • packages/core/package.json
  • packages/rhi-webgl/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/rhi-webgl/src/GLBuffer.ts
  • packages/core/src/graphic/Buffer.ts

Comment on lines +140 to +159
case Array:
let targetPropertyA = <Array<any>>target[k];
const length = (<Array<any>>sourceProperty).length;
if (targetPropertyA == null) {
target[k] = targetPropertyA = new Array<any>(length);
} else {
targetPropertyA.length = length;
}
for (let i = 0; i < length; i++) {
CloneManager.cloneProperty(
<Array<any>>sourceProperty,
targetPropertyA,
i,
cloneMode,
srcRoot,
targetRoot,
deepInstanceMap
);
}
break;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Track arrays in deepInstanceMap before walking them.

The array branch never reuses/registers sourceProperty, so shared arrays split and self-referential arrays recurse forever. Reusing the parent cloneMode for each element also makes @shallowClone arrays deep-clone object items instead of preserving references.

Possible fix
-      case Array:
-        let targetPropertyA = <Array<any>>target[k];
-        const length = (<Array<any>>sourceProperty).length;
-        if (targetPropertyA == null) {
-          target[k] = targetPropertyA = new Array<any>(length);
-        } else {
-          targetPropertyA.length = length;
-        }
-        for (let i = 0; i < length; i++) {
-          CloneManager.cloneProperty(
-            <Array<any>>sourceProperty,
-            targetPropertyA,
-            i,
-            cloneMode,
-            srcRoot,
-            targetRoot,
-            deepInstanceMap
-          );
-        }
-        break;
+      case Array: {
+        let targetPropertyA =
+          (<Array<any>>deepInstanceMap.get(sourceProperty)) ?? <Array<any>>target[k];
+        const length = (<Array<any>>sourceProperty).length;
+        const shouldPopulate = !deepInstanceMap.has(sourceProperty);
+
+        if (targetPropertyA == null) {
+          targetPropertyA = new Array<any>(length);
+        } else {
+          targetPropertyA.length = length;
+        }
+
+        target[k] = targetPropertyA;
+        deepInstanceMap.set(sourceProperty, targetPropertyA);
+
+        if (shouldPopulate) {
+          const itemCloneMode = cloneMode === CloneMode.Shallow ? CloneMode.Assignment : cloneMode;
+          for (let i = 0; i < length; i++) {
+            CloneManager.cloneProperty(
+              <Array<any>>sourceProperty,
+              targetPropertyA,
+              i,
+              itemCloneMode,
+              srcRoot,
+              targetRoot,
+              deepInstanceMap
+            );
+          }
+        }
+        break;
+      }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
case Array:
let targetPropertyA = <Array<any>>target[k];
const length = (<Array<any>>sourceProperty).length;
if (targetPropertyA == null) {
target[k] = targetPropertyA = new Array<any>(length);
} else {
targetPropertyA.length = length;
}
for (let i = 0; i < length; i++) {
CloneManager.cloneProperty(
<Array<any>>sourceProperty,
targetPropertyA,
i,
cloneMode,
srcRoot,
targetRoot,
deepInstanceMap
);
}
break;
case Array: {
let targetPropertyA =
(<Array<any>>deepInstanceMap.get(sourceProperty)) ?? <Array<any>>target[k];
const length = (<Array<any>>sourceProperty).length;
const shouldPopulate = !deepInstanceMap.has(sourceProperty);
if (targetPropertyA == null) {
targetPropertyA = new Array<any>(length);
} else {
targetPropertyA.length = length;
}
target[k] = targetPropertyA;
deepInstanceMap.set(sourceProperty, targetPropertyA);
if (shouldPopulate) {
const itemCloneMode = cloneMode === CloneMode.Shallow ? CloneMode.Assignment : cloneMode;
for (let i = 0; i < length; i++) {
CloneManager.cloneProperty(
<Array<any>>sourceProperty,
targetPropertyA,
i,
itemCloneMode,
srcRoot,
targetRoot,
deepInstanceMap
);
}
}
break;
}
🧰 Tools
🪛 Biome (2.4.7)

[error] 141-141: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.

(lint/correctness/noSwitchDeclarations)


[error] 142-142: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.

(lint/correctness/noSwitchDeclarations)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/clone/CloneManager.ts` around lines 140 - 159, The array
branch fails to register the source array in deepInstanceMap and reuses the
parent cloneMode for every element; before iterating set
deepInstanceMap.set(sourceProperty, targetPropertyA) to register/reuse shared or
self-referential arrays, and compute an element-specific clone mode rather than
reusing the parent cloneMode when calling CloneManager.cloneProperty (e.g.,
derive elementCloneMode from the array's own clone metadata or a dedicated
helper and pass that to CloneManager.cloneProperty instead of cloneMode).

Comment on lines +161 to +188
let targetProperty = <Object>target[k];
// If the target property is undefined, create new instance and keep reference sharing like the source
if (!targetProperty) {
targetProperty = deepInstanceMap.get(sourceProperty);
if (!targetProperty) {
targetProperty = new sourceProperty.constructor();
deepInstanceMap.set(sourceProperty, targetProperty);
}
for (let i = 0; i < length; i++) {
target[k] = targetProperty;
}

if ((<ICustomClone>sourceProperty).copyFrom) {
(<ICustomClone>targetProperty).copyFrom(<ICustomClone>sourceProperty);
} else {
const cloneModes = CloneManager.getCloneMode(sourceProperty.constructor);
for (let k in sourceProperty) {
CloneManager.cloneProperty(
<Array<any>>sourceProperty,
targetPropertyA,
i,
cloneMode,
<Object>sourceProperty,
targetProperty,
k,
cloneModes[k],
srcRoot,
targetRoot,
deepInstanceMap
);
}
break;
default:
let targetProperty = <Object>target[k];
// If the target property is undefined, create new instance and keep reference sharing like the source
if (!targetProperty) {
targetProperty = deepInstanceMap.get(sourceProperty);
if (!targetProperty) {
targetProperty = new sourceProperty.constructor();
deepInstanceMap.set(sourceProperty, targetProperty);
}
target[k] = targetProperty;
}

if ((<ICustomClone>sourceProperty).copyFrom) {
// Custom clone
(<ICustomClone>targetProperty).copyFrom(<ICustomClone>sourceProperty);
} else {
// Universal clone
const cloneModes = CloneManager.getCloneMode(sourceProperty.constructor);
for (let k in sourceProperty) {
CloneManager.cloneProperty(
<Object>sourceProperty,
targetProperty,
k,
cloneModes[k],
srcRoot,
targetRoot,
deepInstanceMap
);
}

// Custom incremental clone
if ((<IComponentCustomClone>sourceProperty)._cloneTo) {
(<IComponentCustomClone>sourceProperty)._cloneTo(
<IComponentCustomClone>targetProperty,
srcRoot,
targetRoot
);
}
}
break;
}
} else {
// null, undefined, primitive type, function
target[k] = sourceProperty;
(<ICustomClone>sourceProperty)._cloneTo?.(<ICustomClone>targetProperty, srcRoot, targetRoot);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Don't recurse a mapped object twice.

When deepInstanceMap already has the source object, this branch still walks it again. A cycle like obj.self = obj will overflow the stack, and aliases to preconstructed target instances won't collapse back to one clone.

Possible fix
-      default:
-        let targetProperty = <Object>target[k];
-        // If the target property is undefined, create new instance and keep reference sharing like the source
-        if (!targetProperty) {
-          targetProperty = deepInstanceMap.get(sourceProperty);
-          if (!targetProperty) {
-            targetProperty = new sourceProperty.constructor();
-            deepInstanceMap.set(sourceProperty, targetProperty);
-          }
-          target[k] = targetProperty;
-        }
-
-        if ((<ICustomClone>sourceProperty).copyFrom) {
-          (<ICustomClone>targetProperty).copyFrom(<ICustomClone>sourceProperty);
-        } else {
-          const cloneModes = CloneManager.getCloneMode(sourceProperty.constructor);
-          for (let k in sourceProperty) {
-            CloneManager.cloneProperty(
-              <Object>sourceProperty,
-              targetProperty,
-              k,
-              cloneModes[k],
-              srcRoot,
-              targetRoot,
-              deepInstanceMap
-            );
-          }
-          (<ICustomClone>sourceProperty)._cloneTo?.(<ICustomClone>targetProperty, srcRoot, targetRoot);
-        }
-        break;
+      default: {
+        let targetProperty = <Object>target[k];
+        const mappedProperty = <Object>deepInstanceMap.get(sourceProperty);
+        const shouldPopulate = !mappedProperty;
+
+        if (mappedProperty) {
+          targetProperty = mappedProperty;
+        } else if (!targetProperty) {
+          targetProperty = new sourceProperty.constructor();
+        }
+
+        target[k] = targetProperty;
+        deepInstanceMap.set(sourceProperty, targetProperty);
+
+        if (shouldPopulate) {
+          if ((<ICustomClone>sourceProperty).copyFrom) {
+            (<ICustomClone>targetProperty).copyFrom(<ICustomClone>sourceProperty);
+          } else {
+            const cloneModes = CloneManager.getCloneMode(sourceProperty.constructor);
+            for (let k in sourceProperty) {
+              CloneManager.cloneProperty(
+                <Object>sourceProperty,
+                targetProperty,
+                k,
+                cloneModes[k],
+                srcRoot,
+                targetRoot,
+                deepInstanceMap
+              );
+            }
+            (<ICustomClone>sourceProperty)._cloneTo?.(<ICustomClone>targetProperty, srcRoot, targetRoot);
+          }
+        }
+        break;
+      }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let targetProperty = <Object>target[k];
// If the target property is undefined, create new instance and keep reference sharing like the source
if (!targetProperty) {
targetProperty = deepInstanceMap.get(sourceProperty);
if (!targetProperty) {
targetProperty = new sourceProperty.constructor();
deepInstanceMap.set(sourceProperty, targetProperty);
}
for (let i = 0; i < length; i++) {
target[k] = targetProperty;
}
if ((<ICustomClone>sourceProperty).copyFrom) {
(<ICustomClone>targetProperty).copyFrom(<ICustomClone>sourceProperty);
} else {
const cloneModes = CloneManager.getCloneMode(sourceProperty.constructor);
for (let k in sourceProperty) {
CloneManager.cloneProperty(
<Array<any>>sourceProperty,
targetPropertyA,
i,
cloneMode,
<Object>sourceProperty,
targetProperty,
k,
cloneModes[k],
srcRoot,
targetRoot,
deepInstanceMap
);
}
break;
default:
let targetProperty = <Object>target[k];
// If the target property is undefined, create new instance and keep reference sharing like the source
if (!targetProperty) {
targetProperty = deepInstanceMap.get(sourceProperty);
if (!targetProperty) {
targetProperty = new sourceProperty.constructor();
deepInstanceMap.set(sourceProperty, targetProperty);
}
target[k] = targetProperty;
}
if ((<ICustomClone>sourceProperty).copyFrom) {
// Custom clone
(<ICustomClone>targetProperty).copyFrom(<ICustomClone>sourceProperty);
} else {
// Universal clone
const cloneModes = CloneManager.getCloneMode(sourceProperty.constructor);
for (let k in sourceProperty) {
CloneManager.cloneProperty(
<Object>sourceProperty,
targetProperty,
k,
cloneModes[k],
srcRoot,
targetRoot,
deepInstanceMap
);
}
// Custom incremental clone
if ((<IComponentCustomClone>sourceProperty)._cloneTo) {
(<IComponentCustomClone>sourceProperty)._cloneTo(
<IComponentCustomClone>targetProperty,
srcRoot,
targetRoot
);
}
}
break;
}
} else {
// null, undefined, primitive type, function
target[k] = sourceProperty;
(<ICustomClone>sourceProperty)._cloneTo?.(<ICustomClone>targetProperty, srcRoot, targetRoot);
}
default: {
let targetProperty = <Object>target[k];
const mappedProperty = <Object>deepInstanceMap.get(sourceProperty);
const shouldPopulate = !mappedProperty;
if (mappedProperty) {
targetProperty = mappedProperty;
} else if (!targetProperty) {
targetProperty = new sourceProperty.constructor();
}
target[k] = targetProperty;
deepInstanceMap.set(sourceProperty, targetProperty);
if (shouldPopulate) {
if ((<ICustomClone>sourceProperty).copyFrom) {
(<ICustomClone>targetProperty).copyFrom(<ICustomClone>sourceProperty);
} else {
const cloneModes = CloneManager.getCloneMode(sourceProperty.constructor);
for (let k in sourceProperty) {
CloneManager.cloneProperty(
<Object>sourceProperty,
targetProperty,
k,
cloneModes[k],
srcRoot,
targetRoot,
deepInstanceMap
);
}
(<ICustomClone>sourceProperty)._cloneTo?.(<ICustomClone>targetProperty, srcRoot, targetRoot);
}
}
break;
}
🧰 Tools
🪛 Biome (2.4.7)

[error] 161-161: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.

(lint/correctness/noSwitchDeclarations)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/clone/CloneManager.ts` around lines 161 - 188, The code
currently continues to recurse even when deepInstanceMap already contains
sourceProperty, causing duplicate traversal and infinite recursion on cycles;
modify the block in CloneManager.cloneProperty so that after fetching
targetProperty = deepInstanceMap.get(sourceProperty) and assigning target[k] =
targetProperty you immediately return (or otherwise skip the subsequent
copy/iteration logic) instead of proceeding to call copyFrom, iterate
properties, or invoke _cloneTo; this ensures when deepInstanceMap has
sourceProperty you reuse the mapped target instance and do not walk the same
source object again.

Comment on lines +175 to +181
const cloneModes = CloneManager.getCloneMode(sourceProperty.constructor);
for (let k in sourceProperty) {
CloneManager.cloneProperty(
<Array<any>>sourceProperty,
targetPropertyA,
i,
cloneMode,
<Object>sourceProperty,
targetProperty,
k,
cloneModes[k],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Propagate CloneMode.Deep into undecorated nested fields.

The recursive call only uses cloneModes[k]. For a @deepClone plain object, nested plain objects/arrays have no field metadata, so they fall back to cloneMode === undefined and get assigned by reference. That makes deep-clone effectively one level deep unless every nested field is separately decorated.

Possible fix
-                cloneModes[k],
+                cloneModes[k] ?? (cloneMode === CloneMode.Deep ? CloneMode.Deep : undefined),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/clone/CloneManager.ts` around lines 175 - 181, The
recursive clone call in CloneManager is not propagating a parent Deep setting
into undecorated nested fields, so in CloneManager.cloneProperty (called from
the loop using sourceProperty/targetProperty and cloneModes[k]) update the logic
to derive an effective mode: if the parent field's mode (from
CloneManager.getCloneMode(sourceProperty.constructor) or the caller's current
mode) is CloneMode.Deep and cloneModes[k] is undefined, treat the effective mode
as CloneMode.Deep before calling CloneManager.cloneProperty; ensure the
effectiveMode (not just cloneModes[k]) is passed down into recursive calls so
nested plain objects/arrays inherit Deep when their parent was annotated.

Comment on lines +148 to 156
const position = Vector3.dot(normal, ray.origin);
// Parallel
if (Math.abs(dir) < zeroTolerance) {
// Check if ray origin is on the plane
if (Math.abs(position + plane.distance) < zeroTolerance) {
return 0;
}
return -1;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find all usages of intersectsRayAndPlane and ray.intersectPlane to assess impact
rg -n "intersectsRayAndPlane|intersectPlane" --type=ts -B2 -A5

# Check for tests covering the parallel ray case
rg -n "parallel|intersectsRayAndPlane" --type=ts -g "*test*" -A10

Repository: galacean/engine

Length of output: 8300


Update JSDoc to reflect parallel ray behavior and verify downstream impact.

This change alters the API contract for intersectsRayAndPlane: parallel rays with their origin on the plane now return 0 (intersection at origin) instead of -1 (no intersection).

Affected code:

  • XRHitTest.ts (line 81): Check if (distance >= 0) will now match parallel rays on planes, treating them as valid hits at distance 0.
  • UIRenderer.ts (line 248): Check if (curDistance >= 0 && curDistance < distance) will similarly detect UI elements when rays are parallel to and lie on them.

Documentation needs updating:

  • CollisionUtil.ts line 141 and Ray.ts line 29 both state "returns -1 otherwise", which no longer covers the parallel-on-plane case returning 0.

Update the JSDoc to clarify this edge case:

- * `@returns` The distance from ray to plane if intersecting, -1 otherwise
+ * `@returns` The distance from ray to plane if intersecting (0 if ray origin lies on plane), -1 otherwise

Verify that treating parallel rays on planes as hits at distance 0 is the intended behavior for XR and UI raycasting.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/math/src/CollisionUtil.ts` around lines 148 - 156, The JSDoc for
intersectsRayAndPlane (in CollisionUtil.ts) and the Ray return description (in
Ray.ts) must be updated to state that when a ray is parallel to the plane but
its origin lies on the plane the function returns 0 (intersection at origin)
instead of -1; update the comments to explicitly describe the parallel-on-plane
edge case and that -1 is returned only when there is no intersection. Also
verify downstream behavior in XRHitTest.ts (the if (distance >= 0) check) and
UIRenderer.ts (the if (curDistance >= 0 && curDistance < distance) check) to
ensure treating parallel-on-plane as a valid hit at distance 0 is
intended—either leave as-is if intended or adjust those conditional checks to
exclude parallel-origin cases, and add/update unit/integration tests to cover
this scenario.

Comment on lines +270 to 275
override _cloneTo(target: Text): void {
// @ts-ignore
super._cloneTo(target, srcRoot, targetRoot);
super._cloneTo(target);
target.font = this._font;
target._subFont = this._subFont;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check how CloneManager handles `@assignmentClone` - does it use setters or direct assignment?
ast-grep --pattern $'case CloneMode.AssignmentClone:
  $$$
'

rg -n -A10 'AssignmentClone' packages/core/src/clone/CloneManager.ts

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

find . -name "CloneManager.ts" -o -name "*clone*.ts" | grep -E "(clone|Clone)" | head -20

Repository: galacean/engine

Length of output: 100


🏁 Script executed:

# First, let's check the Text.ts file to verify the decorators mentioned
cat -n packages/ui/src/component/advanced/Text.ts | head -100

Repository: galacean/engine

Length of output: 3363


🏁 Script executed:

# Find files containing `@assignmentClone` or `@ignoreClone` decorators
rg -l "@assignmentClone|@ignoreClone" --type ts

Repository: galacean/engine

Length of output: 2547


🏁 Script executed:

cat -n packages/core/src/clone/CloneManager.ts | head -150

Repository: galacean/engine

Length of output: 6391


🏁 Script executed:

# Check how the cloning process works - especially the order of operations
rg -n "AssignmentClone" packages/core/src/clone/CloneManager.ts -A 5 -B 2

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Get more of CloneManager.ts to understand the full cloning process
cat -n packages/core/src/clone/CloneManager.ts | tail -100

Repository: galacean/engine

Length of output: 4179


🏁 Script executed:

# Check how ComponentCloner works and when _cloneTo is called relative to automatic cloning
rg -n "_cloneTo\|ComponentCloner" packages/core/src/clone/ -A 3 -B 1

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Look for tests that verify cloning behavior and reference counting
rg -n "clone.*font|font.*clone" tests/ -A 2 -B 1 --type ts -i

Repository: galacean/engine

Length of output: 508


🏁 Script executed:

# Find the complete _cloneTo implementation in Text.ts
cat -n packages/ui/src/component/advanced/Text.ts | sed -n '260,290p'

Repository: galacean/engine

Length of output: 821


🏁 Script executed:

# Check the base class to understand the cloning lifecycle
rg -n "class UIRenderer\|_cloneTo" packages/ui/src/component/UIRenderer.ts -A 5 -B 1 | head -50

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Check the parent class UIRenderer and its _cloneTo implementation
cat -n packages/ui/src/component/UIRenderer.ts | grep -A 20 "_cloneTo"

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Check the Component base class to understand the cloning chain
rg -n "class Component\|_cloneTo" packages/core/src/Component.ts -A 10 -B 2 | head -80

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Verify if there are any other cloning tests that show reference counting behavior
rg -n "clone.*font.*referCount|referCount.*font.*clone" . -i --type ts

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Check what happens with reference counting - look for _addResourceReferCount usage
rg -n "_addResourceReferCount" packages/ui/src/component/advanced/Text.ts -B 2 -A 2

Repository: galacean/engine

Length of output: 495


🏁 Script executed:

# Check the test file more carefully to see if clone tests verify reference counting
cat -n tests/src/ui/Text.test.ts | sed -n '120,145p'

Repository: galacean/engine

Length of output: 736


🏁 Script executed:

# Look at the parent class method to understand what super._cloneTo does
rg -n "class UIRenderer" packages/ui/src/component/UIRenderer.ts -A 100 | grep -A 20 "_cloneTo"

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Let's trace through the complete cloning flow - check if there's a test for reference counting on clone
rg -n "addResourceReferCount\|_referCount" packages/ui/src/component/advanced/Text.ts

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Check parent classes to understand reference counting mechanism
rg -n "addResourceReferCount\|_referCount" packages/core/src/base/EngineObject.ts -B 2 -A 5 | head -50

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Look for any documentation or comments about the interaction between `@assignmentClone` and _cloneTo
rg -n "@assignmentClone.*_cloneTo|_cloneTo.*@assignmentClone" . -i --type ts

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Let's check the actual cloning flow in Entity or Component base class
rg -n "clone.*function\|_cloneProperties" packages/core/src/ --type ts -A 3 -B 1 | head -100

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Verify how the cloning process works by looking at ICustomClone interface
rg -n "interface ICustomClone\|_cloneTo" packages/core/src/clone/ComponentCloner.ts -A 10

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Check if there are integration tests that verify reference counting during clone
cat -n tests/src/ui/Text.test.ts | sed -n '100,150p'

Repository: galacean/engine

Length of output: 1516


🏁 Script executed:

# Let's understand the complete cloning sequence by checking Entity.clone or Component.clone
rg -n "\.clone\(\)" packages/core/src/Entity.ts -A 20 | head -50

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Check if there are any reference counting issues documented or tested
rg -n "referCount|refCount" packages/core/src/base/EngineObject.ts -B 2 -A 5

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Let's trace the actual assignment - check what the direct assignment target[k] = source[k] does
cat -n packages/core/src/clone/CloneManager.ts | sed -n '115,125p'

Repository: galacean/engine

Length of output: 519


🏁 Script executed:

# Now let's verify the actual cloning sequence in Component or Entity
rg -n "cloneProperties\|_cloneProperties" packages/core/src/Entity.ts -B 5 -A 20 | head -80

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Check if there are other similar patterns in the codebase where `@assignmentClone` properties are reassigned in _cloneTo
rg -n "@assignmentClone" packages/ui/src/component/advanced/ -A 30 | grep -B 5 -A 5 "_cloneTo"

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Let's check the Image component to see if it has a similar pattern
cat -n packages/ui/src/component/advanced/Image.ts | grep -A 20 "_cloneTo"

Repository: galacean/engine

Length of output: 886


🏁 Script executed:

# Let's check what decorators Image._sprite has
rg -n "private _sprite" packages/ui/src/component/advanced/Image.ts -B 5 -A 5

Repository: galacean/engine

Length of output: 378


🏁 Script executed:

# Let's also verify the inheritance chain and when _cloneTo is called
cat -n packages/core/src/clone/CloneManager.ts | sed -n '170,195p'

Repository: galacean/engine

Length of output: 1159


🏁 Script executed:

# Check a component with reference counting to see if the pattern is consistent
rg -n "_addResourceReferCount" packages/core/src/Component.ts -B 5 -A 5 | head -40

Repository: galacean/engine

Length of output: 436


🏁 Script executed:

# Now I need to understand if reference counting happens during cloning. Let me check the full initialization flow
cat -n packages/core/src/Component.ts | sed -n '1,50p'

Repository: galacean/engine

Length of output: 1703


🏁 Script executed:

# Key insight: let's verify if CloneManager clones properties from the actual target object or creates a new one
rg -n "clone.*target.*new\|new.*target.*clone" packages/core/src/ --type ts -B 2 -A 3 | head -50

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Most importantly: trace what happens to _font during cloning step by step
# When CloneManager does target[k] = sourceProperty, the target._font is directly set
# Then Text._cloneTo does target.font = this._font through the setter
# But the setter has: if (lastFont !== value) - so if they're the same value, no ref counting happens!
# Let me verify if this is indeed the issue

rg -n "class Text extends" packages/ui/src/component/advanced/Text.ts -A 100 | grep -B 5 -A 15 "constructor\|_onDestroy"

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Key question: when a component is cloned, does the clone process handle reference counting?
# Let me check Entity.clone() to see the full cloning flow
rg -n "clone()" packages/core/src/Entity.ts -B 3 -A 30

Repository: galacean/engine

Length of output: 4921


🏁 Script executed:

# Check if reference counts are managed during cloning by looking at the full clone flow
cat -n packages/core/src/clone/CloneUtils.ts | head -100

Repository: galacean/engine

Length of output: 1982


🏁 Script executed:

# Let me verify: after CloneManager applies `@assignmentClone`, is the ref count incremented elsewhere?
rg -n "cloneComponent\|_cloneProperties" packages/core/src/ --type ts -B 2 -A 15 | head -100

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Now let me check ComponentCloner to see the exact cloning flow
cat -n packages/core/src/clone/ComponentCloner.ts

Repository: galacean/engine

Length of output: 1419


🏁 Script executed:

# Let me verify the exact sequence and understand if reference counting actually happens
# by checking if there's any reference counting during component initialization
rg -n "constructor\|_addResourceReferCount" packages/ui/src/component/advanced/Text.ts | head -20

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Verify whether direct assignment target[k] = source[k] goes through setter or bypasses it
# Let me check if there are other components with similar patterns where _cloneTo reassigns `@assignmentClone` properties
rg -n "override _cloneTo" packages/core/src/ --type ts -A 10 | grep -B 2 -A 8 "@assignmentClone"

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Let me trace through the exact initialization sequence
# Check how components are created during clone - what's the initial state of target._font?
rg -n "new Entity\|new.*Component\|constructor" packages/core/src/Entity.ts | grep -A 2 -B 2 "createCloneEntity\|_components"

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Now let's verify: when CloneManager assigns target._font = source._font (direct assignment),
# and then _cloneTo does target.font = this._font (through setter),
# what happens? The setter checks if (lastFont !== value) but they're the same!

# Let's check if there's reference counting happening elsewhere
rg -n "_addResourceReferCount.*font\|font.*_addResourceReferCount" packages/ui/src/component/advanced/Text.ts

Repository: galacean/engine

Length of output: 41


🏁 Script executed:

# Let me understand the complete lifecycle - is reference counting done when a component is first added?
rg -n "addComponent\|_addResourceReferCount" packages/ui/src/component/UIRenderer.ts -B 2 -A 5 | head -40

Repository: galacean/engine

Length of output: 41


Change _font to @ignoreClone and rely solely on _cloneTo for proper reference counting.

The _font field is decorated with @assignmentClone, which causes CloneManager to directly assign the property without invoking the setter. When _cloneTo subsequently calls target.font = this._font through the setter, the font's reference count is not incremented because the setter's guard condition if (lastFont !== value) evaluates to false (the property was already set to the same value by CloneManager).

This breaks reference counting during cloning. Since _subFont is already @ignoreClone and handled in _cloneTo, apply the same pattern to _font: remove the @assignmentClone decorator and let _cloneTo handle the assignment through the setter, ensuring reference counting is properly invoked.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/ui/src/component/advanced/Text.ts` around lines 270 - 275, Remove
the `@assignmentClone` decorator from the _font field and mark it `@ignoreClone`
like _subFont, then rely on the existing override _cloneTo(target: Text) to
assign via target.font = this._font so the font setter runs and reference
counting occurs; specifically update the _font declaration to use `@ignoreClone`
(remove `@assignmentClone`) and keep the _cloneTo method as-is so the font
setter's guard (in the font property/method) increments/decrements refs
correctly during cloning.

gl.bindBufferBase(gl.TRANSFORM_FEEDBACK_BUFFER, index, null);
}

begin(primitiveMode: number): void {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: begin(primitiveMode: number) 直接将引擎的 MeshTopology 枚举值传给
gl.beginTransformFeedback()。需要确认 MeshTopology 枚举值与 GL 常量对齐(MeshTopology.Points = 0 ===
gl.POINTS = 0)。

// Transform Feedback: render all slots (instance buffer not compacted, dead particles discarded in shader)
// Non-Transform Feedback: render only alive particles (instance buffer compacted)
generator._primitive.instanceCount = generator._useTransformFeedback
? generator._currentParticleCount
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: ParticleRenderer._render() 中 instanceCount 设为 _currentParticleCount 存在性能浪费

ParticleRenderer.ts:236-239:TF 模式下 instanceCount 设为全部 slot 数而非 alive 数量,依赖 shader 中 normalizedAge >= 1.0 来 discard 死粒子。这意味着 大量死粒子 slot 也会执行完整的顶点着色,只是最终被 discard。

* Platform interface for Transform Feedback operations.
* @internal
*/
export interface IPlatformTransformFeedback {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

增加 pauseTransformFeedback() / resumeTransformFeedback()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants